Fixing encodeXmmRegAsIval to ensure the result meets the 'fits in imm8' check #18849
Conversation
|
This is the same as #18820, but in a new PR as the old one seems to have gotten into some weird state that caused the perf correctness jobs to fail (even with a whitespace only change). |
|
@dotnet-bot test Windows_NT x64 Checked jitincompletehwintrinsic please @dotnet-bot test Windows_NT x86 Checked jitincompletehwintrinsic please @dotnet-bot test Ubuntu x64 Checked jitincompletehwintrinsic please |
| @@ -111,7 +111,7 @@ IF_DEF(RRW_RRW_CNS, IS_R1_RW|IS_R2_RW, SCNS) // r/w reg , r/w r | |||
| IF_DEF(RWR_RRD_RRD, IS_R1_WR|IS_R2_RD|IS_R3_RD, NONE) // write reg , read reg2 , read reg3 | |||
| IF_DEF(RWR_RRD_RRD_CNS, IS_R1_WR|IS_R2_RD|IS_R3_RD, SCNS) // write reg , read reg2 , read reg3, const | |||
|
|
|||
| IF_DEF(RWR_RRD_RRD_RRD, IS_R1_WR|IS_R2_RD|IS_R3_RD|IS_R4_RD, NONE) // write reg , read reg2 , read reg3 , read reg4 | |||
| IF_DEF(RWR_RRD_RRD_RRD, IS_R1_WR|IS_R2_RD|IS_R3_RD|IS_R4_RD, CNS) // write reg , read reg2 , read reg3 , read reg4 | |||
There was a problem hiding this comment.
@fiigii, @CarolEidt. This was causing the additional assert, we need to ensure that RWR_RRD_RRD_RRD is marked as carrying a constant (for encoding reg4) so that the debug only id size checks succeed.
This seems to only matter when reg4 is an extended register and generates "large" constant (which is really just a sign-extended byte constant)
| assert(opReg >= XMMBASE); | ||
| int ival = (opReg - XMMBASE) << 4; | ||
|
|
||
| assert((ival >= 0) && (ival <= 255)); |
There was a problem hiding this comment.
But I would at least not cast to a signed type (int8_t) when you're using an unsigned range above.
@CarolEidt, we actually can't do that and both are required.
The JIT does signed check (value must be in range of -128 to 127) for "fits in byte". However, the value returned by the computation, (opReg - XMMBASE) << 4, is within the unsigned range (0 to 255).
If we cast to uint8_t, then the value is zero-extended, rather than sign-extended, and we will fail the "fits in byte" check. This was the same problem that was plaguing the HWIntrinsic APIs that directly take an immediate parameter (as they take a byte). -- I think we also discussed, briefly, that this may cause us to emit code that is larger than necessary in some cases (but I don't see an issue tracking that)...
|
@CarolEidt, do you know who to tag on the Perf Tests Correctness failures? I can't get them to fail locally and it seems to be persistent no matter what the PR contains (it failed last night, on the previous PR with a whitespace only change). |
|
For reference, the jenkins logs are indicating that
|
|
So ... perhaps @adiaaida or @noahfalk who have most recently touched the tests\scripts\run-xunit-perf.py script. |
|
@CarolEidt, @adiaaida, @noahfalk. I found the issue. My PR title contains quotes, which break the |
tannergooding
changed the title
|
@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness please |
|
Logged https://github.com/dotnet/coreclr/issues/18855 to track the issue and changed my PR title in the meantime. |
FYI. @CarolEidt, @fiigii, @eerhardt
This resolves #18815