-
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
Update the x86 hwintrinsic list to match the arm64 layout #35364
Conversation
CC. @echesakovMSFT, @CarolEidt |
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 (minus one suggestion about using curly braces in switch-case-s).
@tannergooding I am planning to remove UnfixedSIMDSize flag - I believe JIT can infer if the SIMD size is arbitrary from -1 in the "SIMD size" column? Do you have any objections?
case NI_SSE2_CompareEqual: | ||
case NI_SSE2_CompareScalarEqual: | ||
case NI_AVX_CompareEqual: | ||
{ |
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.
Why to put curly braces around simple return statement? They are not needed here, they do not increase readability and I don't believe we use them in other places in the JIT. cc @dotnet/jit-contrib
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.
Habit mostly. It minimizes the diff if inserting additional code and allows the block to be collapsed in an IDE.
Sounds fine to me. There are a few places we can improve this further and we can even likely use the |
I had this idea in mind - it would simplify implementation of some of the intrinsics where the simd size controls what instruction to emit (e.g. AddWideningLower (baseType is first arg's type) - for SIMD8 emit addl for SIMD16 emit addw). However, I would postpone such change for now and implement these as SpecialCodeGen . |
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
As commented on #35030 (comment), there are relatively few intrinsics that actually take a hardcoded immediate value on x86 and none on ARM64. Since there are so few intrinsics, roughly 6% (4-bytes) of each
HWIntrinsicInfo
entry was going unused, spread out across the ~480 entries, this was almost 2kb.Given the above, this removes the
ival
entry from the x86 intrinsic list and changes it to be a simple switch table lookup instead. Additionally, since the table entries were being updated anyways, this adjusts the ordering to match what we were doing on ARM64 so that we only need to provide the isa and method name which are used to construct the ID.