-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Rename StoreVectorMxNAndZip to StoreVectorAndZip #103638
Rename StoreVectorMxNAndZip to StoreVectorAndZip #103638
Conversation
Note regarding the
|
1 similar comment
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
Hi @kunalspathak, the coreclr part of this PR is tested but the mono bit is not. There are two commits separating the coreclr and mono changes that would make it easier to undo/update mono changes if needed. |
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.
Added comment to handle it similar to NI_AdvSimd_VectorTableLookup
@fanyang-mono - which CI pipelines we should run to make sure that the code in the PR is getting tested for mono? |
…n hwintrinsiclistarm64.h
The |
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.
looks much simpler. Added few last comments.
unreached(); | ||
} | ||
|
||
GetEmitter()->emitIns_R_R(ins, emitSize, op2Reg, op1Reg, opt); |
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.
can you make sure emitSize == EA_8BYTE
or emitSize == 16BYTE
or do we verify that already by this point?
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.
Sure, it's asserted in genGetSimdInsOpt()
one line 322 above.
We aren't asserting explicitly in other cases, is there a chance getting other values by any chance?
For HW_Category_MemoryStore
, the emitSize
seemed to be pulled from the table in hwintrinsiclistarm64.h.
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.
yes, that's fine.
I am ok with either way. Really depends on if you have cycles to do that change here, or 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.
LGTM coreclr side
…Zip' into swapnil-github-rename-storeVectorNxMAndZip
Test
|
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.
Mono change looks good to me.
/ba-g failures are known or mono amd64, unrelated |
Partially (item 2) fixes: #102340