Minor cleanup in BuildHWIntrinsic to better support adding containment to the existing HWIntrinsic nodes.#18078
Conversation
|
FYI. @fiigii, @CarolEidt |
|
The two biggest issues I saw were:
|
|
@dotnet-bot test Windows_NT x64 Checked jitincompletehwintrinsic @dotnet-bot test Windows_NT x86 Checked jitincompletehwintrinsic @dotnet-bot test Ubuntu x64 Checked jitincompletehwintrinsic |
| } | ||
| else | ||
| { | ||
| assert(numArgs == (op2 == nullptr) ? 1 : 2); |
There was a problem hiding this comment.
This was incorrect for op1 == nullptr
| op2 = op1->AsArgList()->Rest()->Current(); | ||
| op3 = op1->AsArgList()->Rest()->Rest()->Current(); | ||
| op1 = op1->AsArgList()->Current(); | ||
| assert(numArgs >= 3); |
There was a problem hiding this comment.
The assert on L2315 partially invalidates this as it requires numArgs == 3
|
|
||
| if (buildUses) | ||
| { | ||
| if (numArgs > 3) |
There was a problem hiding this comment.
There are currently 0 cases where we have an intrinsic with more than 3 args that would reach the register allocator.
tannergooding
changed the title
|
FYI. @CarolEidt. This has been cleaned up now that #16517 is merged. Could you review when you get the chance? |
| srcCount = 1; | ||
| } | ||
|
|
||
| buildUses = false; |
There was a problem hiding this comment.
This will cause uses not to be built in the case where op1 is a contained memory op. I'm pretty sure that case needs to be handled, so I suspect that the buildUses = false belongs in the if-clause.
| case NI_SSE42_Crc32: | ||
| { | ||
| // TODO - currently we use the BaseType to bring the type of the second argument | ||
| // to the code generator. May encode the overload info in other way. |
There was a problem hiding this comment.
Nit: TODO comments should be qualified - here it should probably be TODO-XArch-Cleanup (see https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-jit-coding-conventions.md#7.1.5). Also, you could expand the last sentence to "We may want to encode the overload info in another way." to be clearer.
|
|
||
| if (!compiler->canUseVexEncoding()) | ||
| { | ||
| assert(isRMW); |
There was a problem hiding this comment.
Moved the assert into the if statement, since it is only RMW on non-VEX hardware.
2 participants
The previous code was making a lot of assumptions about the uselist and the ordering of the LocationInfo in it. I found several of these assumptions to be generally incorrect when working on adding the FMA instruction set.This updates the BuildHWIntrinsic code to be more robust (both against the current code and against future changes) by ensuring that the LocationInfo we are using maps exactly to the operand we think it should.The original comment was invalidated by #16517. The new changes does some minor cleanup to better support some upcoming changes to support containment on the existing HWIntrinsic nodes.