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.
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
Add support for AvxVnni instructions under Experimental. #51998
Add support for AvxVnni instructions under Experimental. #51998
Changes from 1 commit
7865cb8
00a0a0b
83ef75d
12fd6bc
31fd9cf
35bdb57
9526655
e5568b6
e10d641
297f855
eebd83b
5d5f40e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we get a small issue tracking this to ensure it doesn't get lost/forgotten?
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.
Do you want me to open an issue now, or at the time when the code could be merged?
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.
Either is fine, just as long as we are tracking updating it once the official numbers become available.
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.
@tannergooding, #52121 is tracking this.
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.
Thanks!
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.
These assertions are not really needed here - we would check the same invariants at the beginning of
genHWIntrinsic_R_R_R_RM
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.
We aren't doing this for any other HWIntrinsic instructions, instead we generate a
movaps
as required to putop1Reg
intargetReg
.I'd expect register allocation to have already done everything at this point, but I'm not an expert here. @kunalspathak ?
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.
@kunalspathak Does this change looks good to you or could you give me some suggestion on how should we handle it? Thanks.
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 agree with @tannergooding . While I have limited knowledge, but I can see that we generate
movaps
iftargetReg != op1Reg
going forward ingenHWIntrinsic_R_R_R_RM()
. Is there a situation where that doesn't happen and we have to dotargetReg = opt1Reg
explicitely?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 double checked this part of the code and agree that we don't need to do this. I will update the code.
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.
While I was checking this part, I encountered some issue when I set COMPlus_jitDump=RunBasicScenario_Load. The error message I saw is as the following (same error also found in other API such as the ones in Fma_vector256):
Assert failure(PID 5076 [0x000013d4], Thread: 26664 [0x6828]): Assertion failed 'type < CORINFO_TYPE_COUNT' in 'JIT.HardwareIntrinsics.X86.SimpleTernaryOpTest__MultiplyAddDouble:RunBasicScenario_Load():this' during 'Morph - Global' (IL size 137)
@tannergooding, do you know what is happening here?
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'd need to see the full stack trace, but I suspect this would be fixed by merging with (or rebasing onto) the latest dotnet/main.
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 reference, there were a few fixes that went in (#52016, 0e12823, and #51627) resolving asserts that had cropped up related to adding native integer support to the HWIntrinsics code.