Fix recent x86 SIMD regressions #8414
Conversation
1. Recent PUTARG_STK work didn't consider SIMD arguments. 2. SSE3_4 work caused underestimation of instruction sizes for SSE4 instructions (e.g., pmulld).
|
@sivarv @CarolEidt PTAL |
|
Looks good to me. |
| #ifdef _TARGET_AMD64_ | ||
| // If Byte 4 (which is 0xFF00) is non-zero, that's where the RM encoding goes. | ||
|
|
||
| // If Byte 4 (which is 0xFF00) is zero, that's where the RM encoding goes. |
There was a problem hiding this comment.
Oddly, I believe that the previous comment is actually correct, if confusing. The encoding for some reason puts byte 4 in byte 3 (though I would probably have zero-based the counting and called it byte3. Anyway, see instrsxarch.h near line 139 where there's a comment that talks about this.
There was a problem hiding this comment.
You might consider wording it as, "If Byte 4, which is encoded in bits 15-8 of the mask (so we and with 0xFF00)," ...
In reply to: 90478740 [](ancestors = 90478740)
There was a problem hiding this comment.
@CarolEidt The original comment said that if Byte 4 (at mask 0xFF00) is zero, then the size is 5. But that's obviously the opposite of what the code does. My comment change actually reflects what the code does. And I think it reflects that comment in instrsxarch.h as well: in the RM encoding column, we leave the RM slot zero except for the case of 4-byte opcodes that need RM, where all the bytes are already filled with opcode. In that case, we need some special code to determine the RM version size.
It looks like this code is basically a hack for handling the case of a 5-byte opcode + RM in the table, where we don't actually have 5 bytes in the table.
Am I still confused?
There was a problem hiding this comment.
No, you're not confused - I am. It's unfortunate that the comment begins with the opposite of the condition being checked. So NVM (unless you'd like to change the comment to "If Byte 4 (which is 0xFF00) is non-zero, then it will be placed after the 4 byte encoding, making the total 5 bytes. Otherwise, that byte is where the RM encoding goes." Sorry for my confusion.
In reply to: 90501469 [](ancestors = 90501469)
…essions Fix recent x86 SIMD regressions Commit migrated from dotnet/coreclr@eed8ab2
instructions (e.g., pmulld).