Skip to content

Conversation

@tomershafir
Copy link
Contributor

@tomershafir tomershafir commented Nov 4, 2025

NDD ADD is only supported on 64 bit, but LEA32 has Requires<[Not64BitMode]>. The reason it doesnt fail upstream is that the predicates check is commented out on X86MCInstLower.cpp:

  // FIXME: Enable feature predicate checks once all the test pass.
  // X86_MC::verifyInstructionPredicates(MI->getOpcode(),
  //                                     Subtarget->getFeatureBits());

Introduced by: #158254

NDD ADD is only supported on 64 bit, but `LEA32`` has `Requires<[Not64BitMode]>`. The reason it doesnt fail upstream is that the predicates check is commented out on `X86MCInstLower.cpp`:

```
  // FIXME: Enable feature predicate checks once all the test pass.
  // X86_MC::verifyInstructionPredicates(MI->getOpcode(),
  //                                     Subtarget->getFeatureBits());

```

Introduced by: llvm#158254
@tomershafir tomershafir marked this pull request as ready for review November 4, 2025 09:49
@tomershafir tomershafir requested a review from fzou1 November 4, 2025 09:49
@llvmbot
Copy link
Member

llvmbot commented Nov 4, 2025

@llvm/pr-subscribers-backend-x86

Author: Tomer Shafir (tomershafir)

Changes

NDD ADD is only supported on 64 bit, but LEA32`` has Requires<[Not64BitMode]>. The reason it doesnt fail upstream is that the predicates check is commented out on X86MCInstLower.cpp`:

  // FIXME: Enable feature predicate checks once all the test pass.
  // X86_MC::verifyInstructionPredicates(MI-&gt;getOpcode(),
  //                                     Subtarget-&gt;getFeatureBits());

Introduced by: #158254


Full diff: https://github.com/llvm/llvm-project/pull/166334.diff

1 Files Affected:

  • (modified) llvm/lib/Target/X86/X86CompressEVEX.cpp (+1-1)
diff --git a/llvm/lib/Target/X86/X86CompressEVEX.cpp b/llvm/lib/Target/X86/X86CompressEVEX.cpp
index c0c7f5adf06ef..ddbd10d8f7eda 100644
--- a/llvm/lib/Target/X86/X86CompressEVEX.cpp
+++ b/llvm/lib/Target/X86/X86CompressEVEX.cpp
@@ -272,7 +272,7 @@ static bool CompressEVEXImpl(MachineInstr &MI, MachineBasicBlock &MBB,
       const MachineOperand &Src2 = MI.getOperand(2);
       bool Is32BitReg = Opc == X86::ADD32ri_ND || Opc == X86::ADD32rr_ND;
       const MCInstrDesc &NewDesc =
-          ST.getInstrInfo()->get(Is32BitReg ? X86::LEA32r : X86::LEA64r);
+          ST.getInstrInfo()->get(Is32BitReg ? X86::LEA64_32r : X86::LEA64r);
       if (Is32BitReg)
         Src1 = getX86SubSuperRegister(Src1, 64);
       MachineInstrBuilder MIB = BuildMI(MBB, MI, MI.getDebugLoc(), NewDesc, Dst)

@RKSimon
Copy link
Collaborator

RKSimon commented Nov 4, 2025

@tomershafir Are you working towards properly enabling verifyInstructionPredicates ?

@tomershafir
Copy link
Contributor Author

No, I have just seen this failing on tests with the verification enabled

@tomershafir
Copy link
Contributor Author

@RKSimon Feel free to add a review, it would be helpful to merge it fast

Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix!

Copy link
Contributor

@fzou1 fzou1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@tomershafir tomershafir merged commit 3170345 into llvm:main Nov 4, 2025
14 checks passed
@tomershafir tomershafir deleted the x86-fix-ndd-add-32-bit-compression branch November 4, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants