-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT ARM64-SVE: Add FK_3{A,B,C}, EJ_3A, EK_3A, EY_3B, EW_3{A,B} #98187
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e7c7068
Add IF_SVE_FK_3{A,B,C}
amanasifkhalid f8f7143
Add IF_SVE_EJ_3A
amanasifkhalid f7c0820
Add IF_SVE_EK_3A
amanasifkhalid 554b98c
Add IF_SVE_EY_3B
amanasifkhalid 03b3a04
Add IF_SVE_EW_3{A,B} (unsupported)
amanasifkhalid a6686ca
temp
amanasifkhalid ef67c88
Merge from main
amanasifkhalid 4d6ccd8
Fix merge
amanasifkhalid 5516f7d
Add isLowVectorRegister
amanasifkhalid f75c3ce
Fix rotation value
amanasifkhalid 84b3c52
Merge from main
amanasifkhalid 22ccd3f
Merge branch 'main' into sve-sqrdmlah
amanasifkhalid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these instructions that take a rotation value, shouldn't we be passing 0, 90, 180, 270 instead of 0, 1, 2, 3?
I also implemented a few instructions that needed rotation values here: #98141 which I am passing 0, 90, 180, 270.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks a little weird passing 0-3 instead of 0-270, but I did this to match the bit-level representation of the rotation value, so that we don't need a helper method to encode the
rr
bits; then when displaying the instruction in JitDisasms, I multiply the immediate by 90 to display it correctly. I'm fine changing my approach to match yours, though I'll have to update a few encodings already merged in. @kunalspathak @a74nh do you have any preference?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I look at is the
emitIns
functions are APIs that should try to match what the instructions actually are. The bit-level representation/encoding is an implementation detail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In that case, how about I wait for #98141 to be merged in, and then I'll update my encodings that use rotation values to use the helper methods you added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems fair to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we write for readability first if we know that such an optimization would not make any difference in 99.9% of scenarios.
However, I'm fine with encoding it as 0, 1, 2, 3 on
instrDesc
if that is what we all want. I'll have to adjust my work as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is readable from the perspective of calling the
emitIns*
method as seen here: https://github.com/dotnet/runtime/pull/98187/files#diff-d4f9f119d0a321cea7e82023cb754d8abdb800d6185c8bb9464d389ebd50debcR6288and we flip it just before saving it in instrdesc:
https://github.com/dotnet/runtime/pull/98187/files#diff-2b2c8b9011607926410624d6f81613fad7b74c6e0516d578675a8b792998fe4fR11110
I am not sure if
emitOutputInstr()
method is readable anyway :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sympathetic to the readability argument. I guess the silver lining with our current approach is the bitwise representation of the rotation value is abstracted away from the API surface (i.e. the
emitIns
methods). Maybe I'm being naive, but I don't anticipate the code for handling the rotation values inemitIns
oremitDispInsHelp
changing with any frequency after this is merged in, whereas the usage ofemitIns
will certainly increase once we start using these SVE instructions. So in the "important" case, readability isn't hindered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't necessarily about making
emitOutputInstr
readable, but about the display code being simple. We will have to decode theimm
whose values are 0-3 to be translated to 0-270 on display.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I'm trying to say is, if we encode the values as 0-3 on the
instrDesc
, we will have to have anencode
/decode
for it, whereas if we store 0-270, we only need oneencode
function.