Handle a missing case in zero-extend peephole #55129
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
| @@ -296,6 +296,13 @@ bool emitter::AreUpper32BitsZero(regNumber reg) | |||
| return false; | |||
| } | |||
|
|
|||
| #ifdef TARGET_AMD64 | |||
| if (id->idIns() == INS_movsxd) | |||
There was a problem hiding this comment.
This code in general seems error prone given we've introduced a number of non-general purpose instructions (such as BMI1/BMI2) that operate on general purpose registers.
It might be good to log an issue and add a flag to the instruction table indicating whether or not the upper bits are zeroed for a given instruction.
There was a problem hiding this comment.
I think this should only be a problem for instructions with mixed operand sizes. Do any of the BMI instructions mix operand sizes?
There was a problem hiding this comment.
Sorry, this looks to have tagged the wrong line.
I meant AreUpper32BitsZero defaulting to true for any instruction with a "supported format" which deals with a 4-byte size: https://github.com/dotnet/runtime/pull/55129/files/f058b59c7eea2d743d728dc080613e8b6c710e61#diff-6b4e0f32449f2f144e05699f59f74415a564693637f643084a896dfbd081830dR313
// Else rely on operation size.
return (id->idOpSize() == EA_4BYTE);This is basically assuming that any instruction that has a 4BYTE op size zeroes the upper bits and this isn't strictly true for every instruction. I expect its mostly correct due to most machines having the VEX encoding and due to most GPR instructions on 64-bit zeroing the upper bits for the 32-bit version, but it would also be nice if we had this explicitly codified (such as via an instruction flag).
There was a problem hiding this comment.
I do believe that check is right, at least from the perspective of the Intel manual. 3.4.1.1 of the volume on Basic Architecture states:
When in 64-bit mode, operand size determines the number of valid bits in the destination general-purpose
register:
• 64-bit operands generate a 64-bit result in the destination general-purpose register.
• 32-bit operands generate a 32-bit result, zero-extended to a 64-bit result in the destination general-purpose
register.
• 8-bit and 16-bit operands generate an 8-bit or 16-bit result. The upper 56 bits or 48 bits (respectively) of the
destination general-purpose register are not modified by the operation. If the result of an 8-bit or 16-bit
operation is intended for 64-bit address calculation, explicitly sign-extend the register to the full 64-bits.
Perhaps it's more that we get the operand size wrong for these instructions, or that we should have something representing the "destination operand size".
src/tests/JIT/Regression/JitBlue/Runtime_55129/Runtime_55129.cs
Outdated
Show resolved
Hide resolved
jakobbotsch
force-pushed
the
fix-zero-extend-peephole
branch
from
ad0b3bf
to
41044a4
Compare
|
Errors look unrelated. |
ghost
locked as resolved and limited conversation to collaborators
Fix #55127