From 9920f67fd5dd0cf1d64e3b293a085f5957eba344 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 27 Jul 2026 07:13:06 -0700 Subject: [PATCH 1/4] Fix x64 disasm printing 64-bit operands for movzx and GC-typed xor `movzx` never encodes REX.W, and `emitOutputRR` drops it for `xor reg, reg`, but the display forced a pointer-sized destination in both cases -- the latter because a GC type overrode the narrowed operand size. This affected 33,035 instructions in a PMI run over System.Private.CoreLib. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/emitxarch.cpp | 36 +++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 416c99d85d3584..952633ad561818 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -12448,6 +12448,13 @@ void emitter::emitDispIns( } } + // A GC type implies a pointer-sized operand, but the emitter narrows the recorded size when it + // drops REX.W (`xor reg, reg`), so the registers must be named at the size actually encoded. + if (EA_SIZE(attr) != id->idOpSize()) + { + attr = id->idOpSize(); + } + /* Now see what instruction format we've got */ // First print the implicit register usage @@ -12532,10 +12539,15 @@ void emitter::emitDispIns( } else #endif - if (ins == INS_movsx || ins == INS_movzx) + if (ins == INS_movsx) { attr = EA_PTRSIZE; } + else if (ins == INS_movzx) + { + // movzx never encodes REX.W; the 32-bit write already zeroes the upper bits + attr = EA_4BYTE; + } else if ((ins == INS_crc32) && (attr != EA_8BYTE)) { // The idReg1 is always 4 bytes, but the size of idReg2 can vary. @@ -12800,10 +12812,15 @@ void emitter::emitDispIns( } else #endif - if (ins == INS_movsx || ins == INS_movzx) + if (ins == INS_movsx) { attr = EA_PTRSIZE; } + else if (ins == INS_movzx) + { + // movzx never encodes REX.W; the 32-bit write already zeroes the upper bits + attr = EA_4BYTE; + } else if ((ins == INS_crc32) && (attr != EA_8BYTE)) { // The idReg1 is always 4 bytes, but the size of idReg2 can vary. @@ -12983,12 +13000,18 @@ void emitter::emitDispIns( #endif // TARGET_AMD64 case INS_movsx: - case INS_movzx: { tgtAttr = EA_PTRSIZE; break; } + case INS_movzx: + { + // movzx never encodes REX.W; the 32-bit write already zeroes the upper bits + tgtAttr = EA_4BYTE; + break; + } + case INS_crc32: { tgtAttr = std::max(EA_4BYTE, EA_SIZE(attr)); @@ -13377,10 +13400,15 @@ void emitter::emitDispIns( case IF_RWR_MRD: case IF_RRW_MRD: { - if (ins == INS_movsx || ins == INS_movzx) + if (ins == INS_movsx) { attr = EA_PTRSIZE; } + else if (ins == INS_movzx) + { + // movzx never encodes REX.W; the 32-bit write already zeroes the upper bits + attr = EA_4BYTE; + } #ifdef TARGET_AMD64 else if (ins == INS_movsxd) { From 3d6cd430ad6335eecef79d0cef32057c885e20b1 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 27 Jul 2026 07:54:18 -0700 Subject: [PATCH 2/4] Ensure that SIMD disassembly shows the lane selector and correct GPR width `v{extract,insert}{f,i}32x4` were caught by the pseudo-name immediate suppression meant for `cmp*`/`vpcmp*`/`pclmulqdq`, which encode the immediate in the mnemonic. Their pseudo-name is only the VEX-era alias, so the lane selector was dropped -- `vextracti32x4` rendered all four lanes identically. `movmskps`/`movmskpd` named the destination from the vector operand size, giving `rcx` for an encoding that is `REX_WIG` and always writes 32 bits. `pmovmskb` already handled this. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/emitxarch.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 952633ad561818..d5aeaf0b0bd963 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -12229,6 +12229,10 @@ void emitter::emitDispConstant(const instrDesc* id, bool skipComma) const case INS_roundps: case INS_roundsd: case INS_roundss: + case INS_vextractf32x4: + case INS_vextracti32x4: + case INS_vinsertf32x4: + case INS_vinserti32x4: { // These instructions have pseudo-names, but still need to display the immediate break; @@ -12983,6 +12987,8 @@ void emitter::emitDispIns( { switch (ins) { + case INS_movmskpd: + case INS_movmskps: case INS_pmovmskb: { assert(!id->idIsEvexAaaContextSet()); From 80dd1207886cdfa9ba10d0f75cb59608045622b0 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 27 Jul 2026 08:17:33 -0700 Subject: [PATCH 3/4] Ensure that SIMD disassembly names each operand at its own size The narrowing converts only adjusted the destination size in the register form, so the memory forms named it at the source size instead. The scalar integer converts and `vmovd` likewise named their general purpose operand from the vector size. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/emitxarch.cpp | 140 +++++++++++++++++++++++++++------- 1 file changed, 111 insertions(+), 29 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index d5aeaf0b0bd963..6f17cb65f784e6 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -12320,6 +12320,53 @@ void emitter::emitDispConstant(const instrDesc* id, bool skipComma) const emitDispCommentForHandle(cnsVal.cnsVal, id->idDebugOnlyInfo()->idMemCookie, id->idDebugOnlyInfo()->idFlags); } +//------------------------------------------------------------------------ +// GetNarrowedDstAttr: Get the size to display for the destination of an +// instruction that writes fewer bytes than it reads, such as the packed +// double to single converts. +// +// Arguments: +// ins - the instruction +// srcAttr - the size of the source operand +// +// Return Value: +// The size of the destination, or srcAttr when the two match. +// +static emitAttr GetNarrowedDstAttr(instruction ins, emitAttr srcAttr) +{ + switch (ins) + { + case INS_cvtpd2dq: + case INS_cvtpd2ps: + case INS_cvttpd2dq: + case INS_vcvtdq2ph: + case INS_vcvtneps2bf16: + case INS_vcvtpd2udq: + case INS_vcvtps2phx: + case INS_vcvtqq2ps: + case INS_vcvttpd2dqs: + case INS_vcvttpd2udq: + case INS_vcvttpd2udqs: + case INS_vcvtudq2ph: + case INS_vcvtuqq2ps: + { + return std::max(EA_16BYTE, EA_ATTR(EA_SIZE_IN_BYTES(srcAttr) / 2)); + } + + case INS_vcvtpd2ph: + case INS_vcvtqq2ph: + case INS_vcvtuqq2ph: + { + return std::max(EA_16BYTE, EA_ATTR(EA_SIZE_IN_BYTES(srcAttr) / 4)); + } + + default: + { + return srcAttr; + } + } +} + //-------------------------------------------------------------------- // emitDispIns: Dump the given instruction to jitstdout. // @@ -12558,6 +12605,10 @@ void emitter::emitDispIns( // This logic ensures that we print `crc32 eax, bx` instead of `crc32 ax, bx` attr = EA_4BYTE; } + else + { + attr = GetNarrowedDstAttr(ins, attr); + } printf("%s", emitRegName(id->idReg1(), attr)); emitDispEmbMasking(id); printf(", %s", sstr); @@ -12831,6 +12882,10 @@ void emitter::emitDispIns( // This logic ensures that we print `crc32 eax, bx` instead of `crc32 ax, bx` attr = EA_4BYTE; } + else + { + attr = GetNarrowedDstAttr(ins, attr); + } printf("%s", emitRegName(id->idReg1(), attr)); emitDispEmbMasking(id); @@ -13091,6 +13146,8 @@ void emitter::emitDispIns( default: { + tgtAttr = GetNarrowedDstAttr(ins, attr); + switch (ins) { case INS_cvtsi2ss32: @@ -13100,6 +13157,24 @@ void emitter::emitDispIns( { assert(!id->idIsEvexAaaContextSet()); tgtAttr = EA_16BYTE; + srcAttr = EA_ATTR(GetInputSizeInBytes(id)); + break; + } + + case INS_movd32: + case INS_movd64: + { + // Moves between a general purpose register and the low element of a vector + if (isGeneralRegister(id->idReg1())) + { + tgtAttr = EA_ATTR(GetInputSizeInBytes(id)); + srcAttr = EA_16BYTE; + } + else + { + tgtAttr = EA_16BYTE; + srcAttr = EA_ATTR(GetInputSizeInBytes(id)); + } break; } @@ -13147,32 +13222,6 @@ void emitter::emitDispIns( break; } - case INS_cvtpd2dq: - case INS_cvtpd2ps: - case INS_cvttpd2dq: - case INS_vcvtdq2ph: - case INS_vcvtneps2bf16: - case INS_vcvtpd2udq: - case INS_vcvtps2phx: - case INS_vcvtqq2ps: - case INS_vcvttpd2dqs: - case INS_vcvttpd2udq: - case INS_vcvttpd2udqs: - case INS_vcvtudq2ph: - case INS_vcvtuqq2ps: - { - tgtAttr = std::max(EA_16BYTE, EA_ATTR(EA_SIZE_IN_BYTES(attr) / 2)); - break; - } - - case INS_vcvtpd2ph: - case INS_vcvtqq2ph: - case INS_vcvtuqq2ph: - { - tgtAttr = std::max(EA_16BYTE, EA_ATTR(EA_SIZE_IN_BYTES(attr) / 4)); - break; - } - default: break; } @@ -13206,16 +13255,45 @@ void emitter::emitDispIns( std::swap(reg2, reg3); } - emitAttr attr3 = attr; + emitAttr attr12 = attr; + emitAttr attr3 = attr; + if ((insTupleTypeInfo(ins) & INS_TT_MEM128) != 0) { // Shift instructions take xmm for the 3rd operand regardless of instruction size. attr3 = EA_16BYTE; } + else + { + switch (ins) + { + case INS_cvtsi2sd32: + case INS_cvtsi2sd64: + case INS_cvtsi2ss32: + case INS_cvtsi2ss64: + case INS_vcvtsi2sh32: + case INS_vcvtsi2sh64: + case INS_vcvtusi2sd32: + case INS_vcvtusi2sd64: + case INS_vcvtusi2sh32: + case INS_vcvtusi2sh64: + case INS_vcvtusi2ss32: + case INS_vcvtusi2ss64: + { + // The 3rd operand is the general purpose register being converted + attr12 = EA_16BYTE; + attr3 = EA_ATTR(GetInputSizeInBytes(id)); + break; + } + + default: + break; + } + } - printf("%s", emitRegName(id->idReg1(), attr)); + printf("%s", emitRegName(id->idReg1(), attr12)); emitDispEmbMasking(id); - printf(", %s, ", emitRegName(reg2, attr)); + printf(", %s, ", emitRegName(reg2, attr12)); printf("%s", emitRegName(reg3, attr3)); emitDispEmbRounding(id); break; @@ -13427,6 +13505,10 @@ void emitter::emitDispIns( // This logic ensures that we print `crc32 eax, bx` instead of `crc32 ax, bx` attr = EA_4BYTE; } + else + { + attr = GetNarrowedDstAttr(ins, attr); + } printf("%s", emitRegName(id->idReg1(), attr)); emitDispEmbMasking(id); printf(", %s", sstr); From 8946af80fc91b20efdc86d7317b04ac75c11fa35 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 27 Jul 2026 09:16:33 -0700 Subject: [PATCH 4/4] Ensure that broadcast disassembly names the source as xmm The vector-source broadcasts fell to the default case, which names both operands at the destination size. The source is architecturally always xmm. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/emitxarch.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 6f17cb65f784e6..efccd5edd79de2 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -13222,6 +13222,20 @@ void emitter::emitDispIns( break; } + case INS_vbroadcastf32x2: + case INS_vbroadcasti32x2: + case INS_vbroadcastsd: + case INS_vbroadcastss: + case INS_vpbroadcastb: + case INS_vpbroadcastd: + case INS_vpbroadcastq: + case INS_vpbroadcastw: + { + // The source is the low element of a vector, so it is always xmm + srcAttr = EA_16BYTE; + break; + } + default: break; }