[X86] Fix wrong ImmT in several instructions and add assertion#185461
Merged
[X86] Fix wrong ImmT in several instructions and add assertion#185461
Conversation
Several instructions inherited ImmT=Imm8 from *Ii8Base/*Ii8 classes
without having an actual immediate operand. This made
X86MCCodeEmitter::emitMemModRMByte compute ImmSize=1 instead of 0:
int ImmSize = !Disp.isImm() && X86II::hasImm(TSFlags)
? X86II::getSizeOfImm(TSFlags)
: 0;
biasing RIP-relative displacements by one byte (e.g. foo-0x5 instead
of foo-0x4).
Affected instructions:
- VCVTHF82PH (via AVX512XDIi8Base): has memory forms, wrong relocations
- V[U]COMIS{S,D,H}Zrrb (via AVX512P{S,D}Ii8Base): reg-only, latent
- MMX_MOV{DQ2Q,Q2DQ}rr and variants (via MMXSDIi8/MMXS2SIi8): reg-only, latent
Fix by replacing the *Ii8Base/*Ii8 classes with their prefix-only
equivalents (TB, XD, XS, PD) and setting ExeDomain explicitly where
needed.
Add an assertion in encodeInstruction to catch future TSFlags/operand
ImmType mismatches: after consuming all operands in the Form-specific
switch, if `hasImm(TSFlags)` is true but `RemainingOps` is 0 and the Form
isn't one that handles immediates internally (RawFrm, AddCCFrm,
RawFrmImm8, RawFrmImm16, RawFrmMemOffs), the assertion fires. For
example, `VCOMISDZrrb` has `Form=MRMSrcReg` and `ImmT=Imm8` (from
AVX512PDIi8Base), but after consuming both XMM operands CurOp equals
NumOps, leaving RemainingOps=0 with no immediate to emit.
Member
|
@llvm/pr-subscribers-backend-x86 Author: Fangrui Song (MaskRay) ChangesSeveral instructions inherited ImmT=Imm8 from *Ii8Base/*Ii8 classes int ImmSize = !Disp.isImm() && X86II::hasImm(TSFlags) biasing RIP-relative displacements by one byte (e.g. foo-0x5 instead Affected instructions:
Fix by replacing the *Ii8Base/*Ii8 classes with their prefix-only Add an assertion in encodeInstruction to catch future TSFlags/operand Full diff: https://github.com/llvm/llvm-project/pull/185461.diff 5 Files Affected:
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
index cfe5b1094811a..f40e41af516fd 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
@@ -1999,6 +1999,14 @@ void X86MCCodeEmitter::encodeInstruction(const MCInst &MI,
// Skip two trainling conditional operands encoded in EVEX prefix
unsigned RemainingOps = NumOps - CurOp - 2 * HasTwoConditionalOps;
+ // Verify that hasImm(TSFlags) matches the presence of remaining operands.
+ // Exclude forms that emit immediates in the switch above (RawFrm and
+ // AddCCFrm may consume a PC-relative operand; RawFrmImm8/16 and
+ // RawFrmMemOffs always consume their immediates there).
+ assert((!X86II::hasImm(TSFlags) || RemainingOps || Form == X86II::RawFrm ||
+ Form == X86II::AddCCFrm || Form == X86II::RawFrmImm8 ||
+ Form == X86II::RawFrmImm16 || Form == X86II::RawFrmMemOffs) &&
+ "TSFlags indicates immediate but no operand provides it");
while (RemainingOps) {
emitImmediate(MI.getOperand(CurOp++), MI.getLoc(),
getImmFixupKind(Desc.TSFlags),
diff --git a/llvm/lib/Target/X86/X86InstrAVX10.td b/llvm/lib/Target/X86/X86InstrAVX10.td
index 6d388089cd5ca..586923cb6ef3f 100644
--- a/llvm/lib/Target/X86/X86InstrAVX10.td
+++ b/llvm/lib/Target/X86/X86InstrAVX10.td
@@ -970,9 +970,10 @@ multiclass avx10_convert_2op_nomb<string OpcodeStr, AVX512VLVectorVTInfo _dest,
}
}
+let ExeDomain = SSEPackedInt in
defm VCVTHF82PH : avx10_convert_2op_nomb<"vcvthf82ph", avx512vl_f16_info,
avx512vl_i8_info, 0x1e, X86vcvthf82ph>,
- AVX512XDIi8Base, T_MAP5, EVEX, EVEX_CD8<16, CD8VH>;
+ TB, XD, T_MAP5, EVEX, EVEX_CD8<16, CD8VH>;
//-------------------------------------------------
// AVX10 BF16 instructions
diff --git a/llvm/lib/Target/X86/X86InstrAVX512.td b/llvm/lib/Target/X86/X86InstrAVX512.td
index 35e424a1879f3..aafc5cb3c7ee5 100644
--- a/llvm/lib/Target/X86/X86InstrAVX512.td
+++ b/llvm/lib/Target/X86/X86InstrAVX512.td
@@ -9097,13 +9097,13 @@ multiclass avx512_ord_cmp_sae<bits<8> opc, X86VectorVTInfo _,
let Defs = [EFLAGS], Predicates = [HasAVX512] in {
defm VUCOMISSZ : avx512_ord_cmp_sae<0x2E, v4f32x_info, "vucomiss", SSEPackedSingle>,
- AVX512PSIi8Base, EVEX_CD8<32, CD8VT1>;
+ TB, EVEX_CD8<32, CD8VT1>;
defm VUCOMISDZ : avx512_ord_cmp_sae<0x2E, v2f64x_info, "vucomisd", SSEPackedDouble>,
- AVX512PDIi8Base, REX_W, EVEX_CD8<64, CD8VT1>;
+ TB, PD, REX_W, EVEX_CD8<64, CD8VT1>;
defm VCOMISSZ : avx512_ord_cmp_sae<0x2F, v4f32x_info, "vcomiss", SSEPackedSingle>,
- AVX512PSIi8Base, EVEX_CD8<32, CD8VT1>;
+ TB, EVEX_CD8<32, CD8VT1>;
defm VCOMISDZ : avx512_ord_cmp_sae<0x2F, v2f64x_info, "vcomisd", SSEPackedDouble>,
- AVX512PDIi8Base, REX_W, EVEX_CD8<64, CD8VT1>;
+ TB, PD, REX_W, EVEX_CD8<64, CD8VT1>;
}
let Defs = [EFLAGS], Predicates = [HasAVX512] in {
@@ -9138,10 +9138,10 @@ let Defs = [EFLAGS], Predicates = [HasAVX512] in {
let Defs = [EFLAGS], Predicates = [HasFP16] in {
defm VUCOMISHZ : avx512_ord_cmp_sae<0x2E, v8f16x_info, "vucomish",
- SSEPackedSingle>, AVX512PSIi8Base, T_MAP5,
+ SSEPackedSingle>, TB, T_MAP5,
EVEX_CD8<16, CD8VT1>;
defm VCOMISHZ : avx512_ord_cmp_sae<0x2F, v8f16x_info, "vcomish",
- SSEPackedSingle>, AVX512PSIi8Base, T_MAP5,
+ SSEPackedSingle>, TB, T_MAP5,
EVEX_CD8<16, CD8VT1>;
defm VUCOMISHZ : sse12_ord_cmp<0x2E, FR16X, X86any_fcmp, f16, f16mem, loadf16,
"ucomish", SSEPackedSingle>, T_MAP5, EVEX,
diff --git a/llvm/lib/Target/X86/X86InstrMMX.td b/llvm/lib/Target/X86/X86InstrMMX.td
index 644d6d0b92dfc..49f10d036a972 100644
--- a/llvm/lib/Target/X86/X86InstrMMX.td
+++ b/llvm/lib/Target/X86/X86InstrMMX.td
@@ -230,24 +230,28 @@ def MMX_X86movq2dq : SDNode<"X86ISD::MOVQ2DQ", SDTypeProfile<1, 1,
[SDTCisVT<0, v2i64>, SDTCisVT<1, x86mmx>]>>;
let SchedRW = [SchedWriteVecMoveLS.XMM.RR] in {
-def MMX_MOVDQ2Qrr : MMXSDIi8<0xD6, MRMSrcReg, (outs VR64:$dst),
- (ins VR128:$src), "movdq2q\t{$src, $dst|$dst, $src}",
- [(set VR64:$dst,
- (x86mmx (MMX_X86movdq2q VR128:$src)))]>;
-
-def MMX_MOVQ2DQrr : MMXS2SIi8<0xD6, MRMSrcReg, (outs VR128:$dst),
- (ins VR64:$src), "movq2dq\t{$src, $dst|$dst, $src}",
- [(set VR128:$dst,
- (v2i64 (MMX_X86movq2dq VR64:$src)))]>;
+def MMX_MOVDQ2Qrr : I<0xD6, MRMSrcReg, (outs VR64:$dst),
+ (ins VR128:$src), "movdq2q\t{$src, $dst|$dst, $src}",
+ [(set VR64:$dst,
+ (x86mmx (MMX_X86movdq2q VR128:$src)))]>,
+ TB, XD, Requires<[HasMMX, HasSSE2]>;
+
+def MMX_MOVQ2DQrr : I<0xD6, MRMSrcReg, (outs VR128:$dst),
+ (ins VR64:$src), "movq2dq\t{$src, $dst|$dst, $src}",
+ [(set VR128:$dst,
+ (v2i64 (MMX_X86movq2dq VR64:$src)))]>,
+ TB, XS, Requires<[HasMMX, HasSSE2]>;
let isCodeGenOnly = 1, hasSideEffects = 1 in {
-def MMX_MOVQ2FR64rr: MMXS2SIi8<0xD6, MRMSrcReg, (outs FR64:$dst),
- (ins VR64:$src), "movq2dq\t{$src, $dst|$dst, $src}",
- []>;
-
-def MMX_MOVFR642Qrr: MMXSDIi8<0xD6, MRMSrcReg, (outs VR64:$dst),
- (ins FR64:$src), "movdq2q\t{$src, $dst|$dst, $src}",
- []>;
+def MMX_MOVQ2FR64rr: I<0xD6, MRMSrcReg, (outs FR64:$dst),
+ (ins VR64:$src), "movq2dq\t{$src, $dst|$dst, $src}",
+ []>,
+ TB, XS, Requires<[HasMMX, HasSSE2]>;
+
+def MMX_MOVFR642Qrr: I<0xD6, MRMSrcReg, (outs VR64:$dst),
+ (ins FR64:$src), "movdq2q\t{$src, $dst|$dst, $src}",
+ []>,
+ TB, XD, Requires<[HasMMX, HasSSE2]>;
}
} // SchedRW
diff --git a/llvm/test/MC/X86/Relocations/avx10.2satcvt.s b/llvm/test/MC/X86/Relocations/evex-mem.s
similarity index 86%
rename from llvm/test/MC/X86/Relocations/avx10.2satcvt.s
rename to llvm/test/MC/X86/Relocations/evex-mem.s
index 0ace05122d157..69730907b0e50 100644
--- a/llvm/test/MC/X86/Relocations/avx10.2satcvt.s
+++ b/llvm/test/MC/X86/Relocations/evex-mem.s
@@ -25,6 +25,10 @@
# CHECK-NEXT: R_X86_64_PC32 foo-0x4
# CHECK-NEXT: 62 f5 7d 08 6a 05 00 00 00 00 vcvttps2iubs (%rip), %xmm0
# CHECK-NEXT: R_X86_64_PC32 foo-0x4
+# CHECK-NEXT: 62 f5 7f 08 1e 05 00 00 00 00 vcvthf82ph (%rip), %xmm0
+# CHECK-NEXT: R_X86_64_PC32 foo-0x4
+# CHECK-NEXT: 62 f1 7c 08 2e 05 00 00 00 00 vucomiss (%rip), %xmm0
+# CHECK-NEXT: R_X86_64_PC32 foo-0x4
vcvtbf162ibs foo(%rip), %xmm0
vcvtbf162iubs foo(%rip), %xmm0
@@ -38,3 +42,6 @@ vcvttph2ibs foo(%rip), %xmm0
vcvttph2iubs foo(%rip), %xmm0
vcvttps2ibs foo(%rip), %xmm0
vcvttps2iubs foo(%rip), %xmm0
+
+vcvthf82ph foo(%rip), %xmm0
+{evex} vucomiss foo(%rip), %xmm0
|
phoebewang
approved these changes
Mar 10, 2026
Contributor
phoebewang
left a comment
phoebewang
left a comment
There was a problem hiding this comment.
LGTM. Thanks for hardening and fixing this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #185254:
Several instructions inherited ImmT=Imm8 from *Ii8Base/*Ii8 classes
without having an actual immediate operand. This made
X86MCCodeEmitter::emitMemModRMByte compute ImmSize=1 instead of 0:
biasing RIP-relative displacements by one byte (e.g. foo-0x5 instead
of foo-0x4).
Affected instructions:
Fix by replacing the *Ii8Base/*Ii8 classes with their prefix-only
equivalents (TB, XD, XS, PD) and setting ExeDomain explicitly where
needed.
Add an assertion in encodeInstruction to catch future TSFlags/operand
ImmType mismatches: after consuming all operands in the Form-specific
switch, if
hasImm(TSFlags)is true butRemainingOpsis 0 and the Formisn't one that handles immediates internally (RawFrm, AddCCFrm,
RawFrmImm8, RawFrmImm16, RawFrmMemOffs), the assertion fires. For
example,
VCOMISDZrrbhasForm=MRMSrcRegandImmT=Imm8(fromAVX512PDIi8Base), but after consuming both XMM operands CurOp equals
NumOps, leaving RemainingOps=0 with no immediate to emit.