[X86] Add MI-layer routine for getting the index of the first address operand, NFC #78019

nmosier · 2024-01-13T04:36:23Z

Add the MI-layer routine X86::getFirstAddrOperandIdx(), which returns the index of the first address operand of a MachineInstr (or -1 if there is none).

X86II::getMemoryOperandNo(), the existing MC-layer routine used to obtain the index of the first address operand in a 5-operand X86 memory reference, is incomplete: it does not handle pseudo-instructions like TCRETURNmi, resulting in security holes in the mitigation passes that use it (e.g., x86-slh and x86-lvi-load).

X86::getFirstAddrOperandIdx() handles both pseudo and real instructions and is thus more suitable for most use cases than X86II::getMemoryOperandNo(), especially in mitigation passes like x86-slh and x86-lvi-load. For this reason, this patch replaces all uses of X86II::getMemoryOperandNo() with X86::getFirstAddrOperandIdx() in the aforementioned mitigation passes.

llvmbot · 2024-01-13T04:36:51Z

@llvm/pr-subscribers-backend-x86

Author: Nicholas Mosier (nmosier)

Changes

Add the MI-layer routine X86::getFirstAddrOperandIdx(), which returns the index of the first address operand of a MachineInstr (or -1 if there is none).

X86II::getMemoryOperandNo(), the existing MC-layer routine used to obtain the index of the first address operand in a 5-operand X86 memory reference, is incomplete: it does not handle pseudo-instructions like TCRETURNmi, resulting in missed optimizations (e.g., x86-optimize-LEAs)
and security holes (e.g., x86-slh and x86-lvi-load) for the passes that use it.

X86::getFirstAddrOperandIdx() handles both pseudo and real instructions and is thus more suitable for most use cases than X86II::getMemoryOperandNo(). For this reason, this patch replaces most uses of the latter with the former.

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

11 Files Affected:

(modified) llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp (+1-3)
(modified) llvm/lib/Target/X86/X86DiscriminateMemOps.cpp (+1-1)
(modified) llvm/lib/Target/X86/X86DomainReassignment.cpp (+2-7)
(modified) llvm/lib/Target/X86/X86FixupLEAs.cpp (+1-3)
(modified) llvm/lib/Target/X86/X86InsertPrefetch.cpp (+2-4)
(modified) llvm/lib/Target/X86/X86InstrInfo.cpp (+41-11)
(modified) llvm/lib/Target/X86/X86InstrInfo.h (+7)
(modified) llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp (+1-4)
(modified) llvm/lib/Target/X86/X86OptimizeLEAs.cpp (+4-14)
(modified) llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp (+4-16)
(modified) llvm/test/CodeGen/X86/vaargs.ll (+2-1)

diff --git a/llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp b/llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp
index 04931afdec51c3..86614c79c2e69b 100644
--- a/llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp
+++ b/llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp
@@ -289,10 +289,8 @@ static unsigned getYMMtoXMMStoreOpcode(unsigned StoreOpcode) {
 }
 
 static int getAddrOffset(const MachineInstr *MI) {
-  const MCInstrDesc &Descl = MI->getDesc();
-  int AddrOffset = X86II::getMemoryOperandNo(Descl.TSFlags);
+  const int AddrOffset = X86::getFirstAddrOperandIdx(*MI);
   assert(AddrOffset != -1 && "Expected Memory Operand");
-  AddrOffset += X86II::getOperandBias(Descl);
   return AddrOffset;
 }
 
diff --git a/llvm/lib/Target/X86/X86DiscriminateMemOps.cpp b/llvm/lib/Target/X86/X86DiscriminateMemOps.cpp
index becd221e1e86ac..19a6a6bf0713d7 100644
--- a/llvm/lib/Target/X86/X86DiscriminateMemOps.cpp
+++ b/llvm/lib/Target/X86/X86DiscriminateMemOps.cpp
@@ -129,7 +129,7 @@ bool X86DiscriminateMemOps::runOnMachineFunction(MachineFunction &MF) {
   bool Changed = false;
   for (auto &MBB : MF) {
     for (auto &MI : MBB) {
-      if (X86II::getMemoryOperandNo(MI.getDesc().TSFlags) < 0)
+      if (X86::getFirstAddrOperandIdx(MI) < 0)
         continue;
       if (BypassPrefetchInstructions && IsPrefetchOpcode(MI.getDesc().Opcode))
         continue;
diff --git a/llvm/lib/Target/X86/X86DomainReassignment.cpp b/llvm/lib/Target/X86/X86DomainReassignment.cpp
index 20dbaf797e3272..456bd23b5af197 100644
--- a/llvm/lib/Target/X86/X86DomainReassignment.cpp
+++ b/llvm/lib/Target/X86/X86DomainReassignment.cpp
@@ -524,12 +524,10 @@ static bool usedAsAddr(const MachineInstr &MI, Register Reg,
   if (!MI.mayLoadOrStore())
     return false;
 
-  const MCInstrDesc &Desc = TII->get(MI.getOpcode());
-  int MemOpStart = X86II::getMemoryOperandNo(Desc.TSFlags);
+  const int MemOpStart = X86::getFirstAddrOperandIdx(MI);
   if (MemOpStart == -1)
     return false;
 
-  MemOpStart += X86II::getOperandBias(Desc);
   for (unsigned MemOpIdx = MemOpStart,
                 MemOpEnd = MemOpStart + X86::AddrNumOperands;
        MemOpIdx < MemOpEnd; ++MemOpIdx) {
@@ -559,10 +557,7 @@ void X86DomainReassignment::buildClosure(Closure &C, Register Reg) {
     // Do not add registers which are used in address calculation, they will be
     // added to a different closure.
     int OpEnd = DefMI->getNumOperands();
-    const MCInstrDesc &Desc = DefMI->getDesc();
-    int MemOp = X86II::getMemoryOperandNo(Desc.TSFlags);
-    if (MemOp != -1)
-      MemOp += X86II::getOperandBias(Desc);
+    const int MemOp = X86::getFirstAddrOperandIdx(*DefMI);
     for (int OpIdx = 0; OpIdx < OpEnd; ++OpIdx) {
       if (OpIdx == MemOp) {
         // skip address calculation.
diff --git a/llvm/lib/Target/X86/X86FixupLEAs.cpp b/llvm/lib/Target/X86/X86FixupLEAs.cpp
index beeebf42dfe81a..3c9d4326e8d9ce 100644
--- a/llvm/lib/Target/X86/X86FixupLEAs.cpp
+++ b/llvm/lib/Target/X86/X86FixupLEAs.cpp
@@ -650,10 +650,8 @@ void FixupLEAPass::processInstruction(MachineBasicBlock::iterator &I,
                                       MachineBasicBlock &MBB) {
   // Process a load, store, or LEA instruction.
   MachineInstr &MI = *I;
-  const MCInstrDesc &Desc = MI.getDesc();
-  int AddrOffset = X86II::getMemoryOperandNo(Desc.TSFlags);
+  const int AddrOffset = X86::getFirstAddrOperandIdx(MI);
   if (AddrOffset >= 0) {
-    AddrOffset += X86II::getOperandBias(Desc);
     MachineOperand &p = MI.getOperand(AddrOffset + X86::AddrBaseReg);
     if (p.isReg() && p.getReg() != X86::ESP) {
       seekLEAFixup(p, I, MBB);
diff --git a/llvm/lib/Target/X86/X86InsertPrefetch.cpp b/llvm/lib/Target/X86/X86InsertPrefetch.cpp
index 9aa70dff5f9327..00ee7d465210fe 100644
--- a/llvm/lib/Target/X86/X86InsertPrefetch.cpp
+++ b/llvm/lib/Target/X86/X86InsertPrefetch.cpp
@@ -200,11 +200,9 @@ bool X86InsertPrefetch::runOnMachineFunction(MachineFunction &MF) {
       auto Current = MI;
       ++MI;
 
-      int Offset = X86II::getMemoryOperandNo(Current->getDesc().TSFlags);
-      if (Offset < 0)
+      const int MemOpOffset = X86::getFirstAddrOperandIdx(*Current);
+      if (MemOpOffset < 0)
         continue;
-      unsigned Bias = X86II::getOperandBias(Current->getDesc());
-      int MemOpOffset = Offset + Bias;
       // FIXME(mtrofin): ORE message when the recommendation cannot be taken.
       if (!IsMemOpCompatibleWithPrefetch(*Current, MemOpOffset))
         continue;
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp
index bddda6891356e8..acfec1a3072742 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -3463,6 +3463,44 @@ bool X86::isX87Instruction(MachineInstr &MI) {
   return false;
 }
 
+int X86::getFirstAddrOperandIdx(const MachineInstr &MI) {
+  const auto isMemOp = [](const MCOperandInfo &OpInfo) -> bool {
+    return OpInfo.OperandType == MCOI::OPERAND_MEMORY;
+  };
+
+  const MCInstrDesc &Desc = MI.getDesc();
+
+  // An instruction cannot have a memory reference if it has fewer than
+  // AddrNumOperands (= 5) explicit operands.
+  if (Desc.getNumOperands() < X86::AddrNumOperands) {
+    assert(none_of(Desc.operands(), isMemOp) &&
+           "Expected no operands to have OPERAND_MEMORY type!");
+    return -1;
+  }
+
+  // The first operand with type OPERAND_MEMORY indicates the start of a memory
+  // reference. We expect the following AddrNumOperand-1 operands to also have
+  // OPERAND_MEMORY type.
+  for (unsigned i = 0; i <= Desc.getNumOperands() - X86::AddrNumOperands; ++i) {
+    if (Desc.operands()[i].OperandType == MCOI::OPERAND_MEMORY) {
+      assert(std::all_of(Desc.operands().begin() + i,
+                         Desc.operands().begin() + i + X86::AddrNumOperands,
+                         isMemOp) &&
+             "Expected all five operands in the memory reference to have "
+             "OPERAND_MEMORY type!");
+      return i;
+    }
+  }
+
+  // Fall back to the MC-layer routine, which only handles real (not pseudo)
+  // insturctions.
+  const int FallbackMemOpNo = X86II::getMemoryOperandNo(Desc.TSFlags);
+  if (FallbackMemOpNo >= 0)
+    return FallbackMemOpNo + X86II::getOperandBias(Desc);
+
+  return -1;
+}
+
 bool X86InstrInfo::isUnconditionalTailCall(const MachineInstr &MI) const {
   switch (MI.getOpcode()) {
   case X86::TCRETURNdi:
@@ -3723,10 +3761,8 @@ bool X86InstrInfo::analyzeBranch(MachineBasicBlock &MBB,
 }
 
 static int getJumpTableIndexFromAddr(const MachineInstr &MI) {
-  const MCInstrDesc &Desc = MI.getDesc();
-  int MemRefBegin = X86II::getMemoryOperandNo(Desc.TSFlags);
+  const int MemRefBegin = X86::getFirstAddrOperandIdx(MI);
   assert(MemRefBegin >= 0 && "instr should have memory operand");
-  MemRefBegin += X86II::getOperandBias(Desc);
 
   const MachineOperand &MO = MI.getOperand(MemRefBegin + X86::AddrDisp);
   if (!MO.isJTI())
@@ -4321,13 +4357,10 @@ static unsigned getLoadStoreRegOpcode(Register Reg,
 std::optional<ExtAddrMode>
 X86InstrInfo::getAddrModeFromMemoryOp(const MachineInstr &MemI,
                                       const TargetRegisterInfo *TRI) const {
-  const MCInstrDesc &Desc = MemI.getDesc();
-  int MemRefBegin = X86II::getMemoryOperandNo(Desc.TSFlags);
+  const int MemRefBegin = X86::getFirstAddrOperandIdx(MemI);
   if (MemRefBegin < 0)
     return std::nullopt;
 
-  MemRefBegin += X86II::getOperandBias(Desc);
-
   auto &BaseOp = MemI.getOperand(MemRefBegin + X86::AddrBaseReg);
   if (!BaseOp.isReg()) // Can be an MO_FrameIndex
     return std::nullopt;
@@ -4448,13 +4481,10 @@ bool X86InstrInfo::getMemOperandsWithOffsetWidth(
     const MachineInstr &MemOp, SmallVectorImpl<const MachineOperand *> &BaseOps,
     int64_t &Offset, bool &OffsetIsScalable, unsigned &Width,
     const TargetRegisterInfo *TRI) const {
-  const MCInstrDesc &Desc = MemOp.getDesc();
-  int MemRefBegin = X86II::getMemoryOperandNo(Desc.TSFlags);
+  const int MemRefBegin = X86::getFirstAddrOperandIdx(MemOp);
   if (MemRefBegin < 0)
     return false;
 
-  MemRefBegin += X86II::getOperandBias(Desc);
-
   const MachineOperand *BaseOp =
       &MemOp.getOperand(MemRefBegin + X86::AddrBaseReg);
   if (!BaseOp->isReg()) // Can be an MO_FrameIndex
diff --git a/llvm/lib/Target/X86/X86InstrInfo.h b/llvm/lib/Target/X86/X86InstrInfo.h
index eb0734f9a61824..75d20b3ef9eb5c 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.h
+++ b/llvm/lib/Target/X86/X86InstrInfo.h
@@ -77,6 +77,13 @@ unsigned getSwappedVCMPImm(unsigned Imm);
 
 /// Check if the instruction is X87 instruction.
 bool isX87Instruction(MachineInstr &MI);
+
+/// Return the index of the instruction's first address operand, if it has a
+/// memory reference, or -1 if it has none. Unlike X86II::getMemoryOperandNo(),
+/// this also works for both pseudo instructions (e.g., TCRETURNmi) as well as
+/// real instructions (e.g., JMP64m).
+int getFirstAddrOperandIdx(const MachineInstr &MI);
+
 } // namespace X86
 
 /// isGlobalStubReference - Return true if the specified TargetFlag operand is
diff --git a/llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp b/llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp
index 2e492fa9c5eead..4dfe7556df0030 100644
--- a/llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp
+++ b/llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp
@@ -770,16 +770,13 @@ bool X86LoadValueInjectionLoadHardeningPass::instrUsesRegToAccessMemory(
       MI.getOpcode() == X86::SFENCE || MI.getOpcode() == X86::LFENCE)
     return false;
 
-  // FIXME: This does not handle pseudo loading instruction like TCRETURN*
-  const MCInstrDesc &Desc = MI.getDesc();
-  int MemRefBeginIdx = X86II::getMemoryOperandNo(Desc.TSFlags);
+  const int MemRefBeginIdx = X86::getFirstAddrOperandIdx(MI);
   if (MemRefBeginIdx < 0) {
     LLVM_DEBUG(dbgs() << "Warning: unable to obtain memory operand for loading "
                          "instruction:\n";
                MI.print(dbgs()); dbgs() << '\n';);
     return false;
   }
-  MemRefBeginIdx += X86II::getOperandBias(Desc);
 
   const MachineOperand &BaseMO =
       MI.getOperand(MemRefBeginIdx + X86::AddrBaseReg);
diff --git a/llvm/lib/Target/X86/X86OptimizeLEAs.cpp b/llvm/lib/Target/X86/X86OptimizeLEAs.cpp
index 3172896a8f6092..819339923ded63 100644
--- a/llvm/lib/Target/X86/X86OptimizeLEAs.cpp
+++ b/llvm/lib/Target/X86/X86OptimizeLEAs.cpp
@@ -341,8 +341,7 @@ bool X86OptimizeLEAPass::chooseBestLEA(
     MachineInstr *&BestLEA, int64_t &AddrDispShift, int &Dist) {
   const MachineFunction *MF = MI.getParent()->getParent();
   const MCInstrDesc &Desc = MI.getDesc();
-  int MemOpNo = X86II::getMemoryOperandNo(Desc.TSFlags) +
-                X86II::getOperandBias(Desc);
+  const int MemOpNo = X86::getFirstAddrOperandIdx(MI);
 
   BestLEA = nullptr;
 
@@ -443,16 +442,13 @@ bool X86OptimizeLEAPass::isReplaceable(const MachineInstr &First,
     MachineInstr &MI = *MO.getParent();
 
     // Get the number of the first memory operand.
-    const MCInstrDesc &Desc = MI.getDesc();
-    int MemOpNo = X86II::getMemoryOperandNo(Desc.TSFlags);
+    const int MemOpNo = X86::getFirstAddrOperandIdx(MI);
 
     // If the use instruction has no memory operand - the LEA is not
     // replaceable.
     if (MemOpNo < 0)
       return false;
 
-    MemOpNo += X86II::getOperandBias(Desc);
-
     // If the address base of the use instruction is not the LEA def register -
     // the LEA is not replaceable.
     if (!isIdenticalOp(MI.getOperand(MemOpNo + X86::AddrBaseReg), MO))
@@ -507,15 +503,12 @@ bool X86OptimizeLEAPass::removeRedundantAddrCalc(MemOpMap &LEAs) {
       continue;
 
     // Get the number of the first memory operand.
-    const MCInstrDesc &Desc = MI.getDesc();
-    int MemOpNo = X86II::getMemoryOperandNo(Desc.TSFlags);
+    const int MemOpNo = X86::getFirstAddrOperandIdx(MI);
 
     // If instruction has no memory operand - skip it.
     if (MemOpNo < 0)
       continue;
 
-    MemOpNo += X86II::getOperandBias(Desc);
-
     // Do not call chooseBestLEA if there was no matching LEA
     auto Insns = LEAs.find(getMemOpKey(MI, MemOpNo));
     if (Insns == LEAs.end())
@@ -668,10 +661,7 @@ bool X86OptimizeLEAPass::removeRedundantLEAs(MemOpMap &LEAs) {
           }
 
           // Get the number of the first memory operand.
-          const MCInstrDesc &Desc = MI.getDesc();
-          int MemOpNo =
-              X86II::getMemoryOperandNo(Desc.TSFlags) +
-              X86II::getOperandBias(Desc);
+          const int MemOpNo = X86::getFirstAddrOperandIdx(MI);
 
           // Update address base.
           MO.setReg(FirstVReg);
diff --git a/llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp b/llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
index 6301285fe95457..ce03432fd8cc23 100644
--- a/llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
+++ b/llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
@@ -1317,12 +1317,7 @@ void X86SpeculativeLoadHardeningPass::tracePredStateThroughBlocksAndHarden(
           continue;
 
         // Extract the memory operand information about this instruction.
-        // FIXME: This doesn't handle loading pseudo instructions which we often
-        // could handle with similarly generic logic. We probably need to add an
-        // MI-layer routine similar to the MC-layer one we use here which maps
-        // pseudos much like this maps real instructions.
-        const MCInstrDesc &Desc = MI.getDesc();
-        int MemRefBeginIdx = X86II::getMemoryOperandNo(Desc.TSFlags);
+        const int MemRefBeginIdx = X86::getFirstAddrOperandIdx(MI);
         if (MemRefBeginIdx < 0) {
           LLVM_DEBUG(dbgs()
                          << "WARNING: unable to harden loading instruction: ";
@@ -1330,8 +1325,6 @@ void X86SpeculativeLoadHardeningPass::tracePredStateThroughBlocksAndHarden(
           continue;
         }
 
-        MemRefBeginIdx += X86II::getOperandBias(Desc);
-
         MachineOperand &BaseMO =
             MI.getOperand(MemRefBeginIdx + X86::AddrBaseReg);
         MachineOperand &IndexMO =
@@ -1365,7 +1358,7 @@ void X86SpeculativeLoadHardeningPass::tracePredStateThroughBlocksAndHarden(
         // could prune out subsequent loads.
         if (EnablePostLoadHardening && X86InstrInfo::isDataInvariantLoad(MI) &&
             !isEFLAGSDefLive(MI) && MI.getDesc().getNumDefs() == 1 &&
-            MI.getOperand(0).isReg() &&
+            MI.getOperand(0).isReg() && MI.getOperand(0).getReg().isVirtual() &&
             canHardenRegister(MI.getOperand(0).getReg()) &&
             !HardenedAddrRegs.count(BaseReg) &&
             !HardenedAddrRegs.count(IndexReg)) {
@@ -1400,12 +1393,9 @@ void X86SpeculativeLoadHardeningPass::tracePredStateThroughBlocksAndHarden(
 
         // Check if this is a load whose address needs to be hardened.
         if (HardenLoadAddr.erase(&MI)) {
-          const MCInstrDesc &Desc = MI.getDesc();
-          int MemRefBeginIdx = X86II::getMemoryOperandNo(Desc.TSFlags);
+          const int MemRefBeginIdx = X86::getFirstAddrOperandIdx(MI);
           assert(MemRefBeginIdx >= 0 && "Cannot have an invalid index here!");
 
-          MemRefBeginIdx += X86II::getOperandBias(Desc);
-
           MachineOperand &BaseMO =
               MI.getOperand(MemRefBeginIdx + X86::AddrBaseReg);
           MachineOperand &IndexMO =
@@ -1802,11 +1792,9 @@ MachineInstr *X86SpeculativeLoadHardeningPass::sinkPostLoadHardenedInst(
 
         // Otherwise, this is a load and the load component can't be data
         // invariant so check how this register is being used.
-        const MCInstrDesc &Desc = UseMI.getDesc();
-        int MemRefBeginIdx = X86II::getMemoryOperandNo(Desc.TSFlags);
+        const int MemRefBeginIdx = X86::getFirstAddrOperandIdx(UseMI);
         assert(MemRefBeginIdx >= 0 &&
                "Should always have mem references here!");
-        MemRefBeginIdx += X86II::getOperandBias(Desc);
 
         MachineOperand &BaseMO =
             UseMI.getOperand(MemRefBeginIdx + X86::AddrBaseReg);
diff --git a/llvm/test/CodeGen/X86/vaargs.ll b/llvm/test/CodeGen/X86/vaargs.ll
index dc1c5d97716ba9..60c1070984f3b1 100644
--- a/llvm/test/CodeGen/X86/vaargs.ll
+++ b/llvm/test/CodeGen/X86/vaargs.ll
@@ -1,4 +1,5 @@
-; RUN: llc -verify-machineinstrs -mcpu=corei7-avx %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=NO-FLAGS
+; RUN: llc -verify-machineinstrs -mcpu=corei7-avx -disable-x86-lea-opt %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=NO-FLAGS
+; Disable the x86-optimize-LEAs pass, since it may introduce an intermediate LEA instruction that changes the base register of the vmovaps instructions from %rsp to something else.
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"

phoebewang · 2024-01-13T10:41:01Z

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp

@@ -1365,7 +1358,7 @@ void X86SpeculativeLoadHardeningPass::tracePredStateThroughBlocksAndHarden(
        // could prune out subsequent loads.
        if (EnablePostLoadHardening && X86InstrInfo::isDataInvariantLoad(MI) &&
            !isEFLAGSDefLive(MI) && MI.getDesc().getNumDefs() == 1 &&
-            MI.getOperand(0).isReg() &&
+            MI.getOperand(0).isReg() && MI.getOperand(0).getReg().isVirtual() &&


Do not put unrelated change in one patch.

Removed. That got in there by mistake (it's from a different patch).

phoebewang · 2024-01-13T10:41:59Z

llvm/test/CodeGen/X86/vaargs.ll

+; RUN: llc -verify-machineinstrs -mcpu=corei7-avx -disable-x86-lea-opt %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=NO-FLAGS
+; Disable the x86-optimize-LEAs pass, since it may introduce an intermediate LEA instruction that changes the base register of the vmovaps instructions from %rsp to something else.


Why do we need the change here? I understand the patch is NFC.

I reverted the changes in X86OptimizeLEAs.cpp (it uses X86II::getMemoryOperandNo again) and the changes to this test. It looks like X86OptimizeLEAs can't handle pseudo instructions like VASTART_SAVE_XMM_REGS that may expand to multiple memory accesses at different displacements.

phoebewang · 2024-01-13T10:43:49Z

llvm/lib/Target/X86/X86DiscriminateMemOps.cpp

@@ -129,7 +129,7 @@ bool X86DiscriminateMemOps::runOnMachineFunction(MachineFunction &MF) {
  bool Changed = false;
  for (auto &MBB : MF) {
    for (auto &MI : MBB) {
-      if (X86II::getMemoryOperandNo(MI.getDesc().TSFlags) < 0)
+      if (X86::getFirstAddrOperandIdx(MI) < 0)


Maybe still use getMemoryOperandNo?

phoebewang

LGTM except one nit.

phoebewang · 2024-01-14T01:40:32Z

llvm/lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp

@@ -289,10 +289,8 @@ static unsigned getYMMtoXMMStoreOpcode(unsigned StoreOpcode) {
 }

 static int getAddrOffset(const MachineInstr *MI) {
-  const MCInstrDesc &Descl = MI->getDesc();
-  int AddrOffset = X86II::getMemoryOperandNo(Descl.TSFlags);
+  const int AddrOffset = X86::getFirstAddrOperandIdx(*MI);


The const doesn't work here. Change the function to static const int or remove it.

Is it just a stylstic preference? Or why doesn't declaring AddrOffset as const work here?

We usually don't use const for integer tpye. I think you may want to highlight AddrOffset is the final offset with bias and don't change it anymore by using const. In that way, it doesn't work here, because the function doesn't consume AddrOffset but return it in a non-const int.

Thanks, fixed.

KanRobert · 2024-01-15T03:14:32Z

it does not handle pseudo-instructions like TCRETURNmi, resulting in missed optimizations (e.g., x86-optimize-LEAs)

Why there is no test update for this?

phoebewang · 2024-01-15T03:31:31Z

it does not handle pseudo-instructions like TCRETURNmi, resulting in missed optimizations (e.g., x86-optimize-LEAs)

Why there is no test update for this?

The change was reverted, but the description doesn't update.

KanRobert · 2024-01-15T03:41:11Z

llvm/lib/Target/X86/X86InstrInfo.cpp

+int X86::getFirstAddrOperandIdx(const MachineInstr &MI) {
+  const auto isMemOp = [](const MCOperandInfo &OpInfo) -> bool {
+    return OpInfo.OperandType == MCOI::OPERAND_MEMORY;
+  };
+
+  const MCInstrDesc &Desc = MI.getDesc();
+
+  // An instruction cannot have a memory reference if it has fewer than
+  // AddrNumOperands (= 5) explicit operands.
+  if (Desc.getNumOperands() < X86::AddrNumOperands) {
+    assert(none_of(Desc.operands(), isMemOp) &&
+           "Expected no operands to have OPERAND_MEMORY type!");
+    return -1;
+  }
+
+  // The first operand with type OPERAND_MEMORY indicates the start of a memory
+  // reference. We expect the following AddrNumOperand-1 operands to also have
+  // OPERAND_MEMORY type.
+  for (unsigned i = 0; i <= Desc.getNumOperands() - X86::AddrNumOperands; ++i) {
+    if (Desc.operands()[i].OperandType == MCOI::OPERAND_MEMORY) {
+      assert(std::all_of(Desc.operands().begin() + i,
+                         Desc.operands().begin() + i + X86::AddrNumOperands,
+                         isMemOp) &&
+             "Expected all five operands in the memory reference to have "
+             "OPERAND_MEMORY type!");
+      return i;
+    }
+  }
+
+  // Fall back to the MC-layer routine, which only handles real (not pseudo)
+  // insturctions.
+  const int FallbackMemOpNo = X86II::getMemoryOperandNo(Desc.TSFlags);
+  if (FallbackMemOpNo >= 0)
+    return FallbackMemOpNo + X86II::getOperandBias(Desc);
+
+  return -1;
+}


This implementation has some flaws

It increases compile-time for non-pseduo instructions

It delays the time when the bug (wrong format) is exposed

Couldn't we can check pseduo and fall back to MC-layer first?

KanRobert · 2024-01-15T03:45:19Z

it does not handle pseudo-instructions like TCRETURNmi, resulting in missed optimizations (e.g., x86-optimize-LEAs)

Why there is no test update for this?

The change was reverted, but the description doesn't update.

So it does allow more optimization to happen?

… operand Add the MI-layer routine X86::getFirstAddrOperandIdx(), which returns the index of the first address operand of a MachineInstr (or -1 if there is none). X86II::getMemoryOperandNo(), the existing MC-layer routine used to obtain the index of the first address operand in a 5-operand X86 memory reference, is incomplete: it does not handle pseudo-instructions like TCRETURNmi, resulting in missed optimizations (e.g., x86-optimize-LEAs) and security holes (e.g., x86-slh and x86-lvi-load) for the passes that use it. X86::getFirstAddrOperandIdx() handles both pseudo and real instructions and is thus more suitable for most use cases than X86II::getMemoryOperandNo(). For this reason, this patch replaces most uses of the latter with the former.

Revert X86DiscriminateMemOps, X86OptimizeLEAs. Remove extraneous change in X86SpeculativeLoadHardening. Revert vaargs.ll test, since X86OptimizeLEAs was reverted.

nmosier · 2024-01-15T21:33:06Z

I think that it does in principle allow more optimizations, for example in the x86-optimize-LEAs pass. However, I found that there need to be a couple tweaks in the x86-optimize-LEAs implementation to ensure that it doesn't introduce anti-optimizations as well.

For example, some pseudo-instructions like VASTART_SAVE_XMM_REGS are expanded to multiple memory accesses at different displacements, which x86-optimize-LEAs does not realize and naively inserts a LEA instruction that doesn't actually help in the end. This should be easy to fix, but I am not planning on addressing it in this patch.

Therefore, I'm reverting all uses of X86II::getMemoryOperandNo() in optimization passes. However, I'm keeping X86::getFirstAddrOperandIdx() in all mitigation passes, since this is necessary for their security guarantees.

I've updated the PR comment to reflect this and just pushed a new commit with these changes.

I am planning on later submitting some follow-up patches to migrate uses of X86II::getMemoryOperandNo() to X86::getFirstAddrOperandIdx() where appropriate in x86 optimization passes. I'll include new tests for those.

KanRobert · 2024-01-16T02:06:23Z

llvm/lib/Target/X86/X86InstrInfo.cpp

+    assert(none_of(Desc.operands(), isMemOp) &&
+           "Got false negative from X86II::getMemoryOperandNo()!");


Wrap these in

#ifdef EXPENSIVE_CHECKS
#endif

KanRobert · 2024-01-16T02:06:34Z

llvm/lib/Target/X86/X86InstrInfo.cpp

+    assert(none_of(Desc.operands(), isMemOp) &&
+           "Expected no operands to have OPERAND_MEMORY type!");


Wrap these in

#ifdef EXPENSIVE_CHECKS
#endif

llvm/lib/Target/X86/X86InstrInfo.cpp

KanRobert

LGTM

nmosier · 2024-01-16T02:50:21Z

Thanks! Can you merge it (as I don't have commit access)?

KanRobert · 2024-01-16T02:54:18Z

Thanks! Can you merge it (as I don't have commit access)?

Sure

BTW, I adjust the code by LLVM coding standards.

… operand, NFC (llvm#78019) Add the MI-layer routine X86::getFirstAddrOperandIdx(), which returns the index of the first address operand of a MachineInstr (or -1 if there is none). X86II::getMemoryOperandNo(), the existing MC-layer routine used to obtain the index of the first address operand in a 5-operand X86 memory reference, is incomplete: it does not handle pseudo-instructions like TCRETURNmi, resulting in security holes in the mitigation passes that use it (e.g., x86-slh and x86-lvi-load). X86::getFirstAddrOperandIdx() handles both pseudo and real instructions and is thus more suitable for most use cases than X86II::getMemoryOperandNo(), especially in mitigation passes like x86-slh and x86-lvi-load. For this reason, this patch replaces all uses of X86II::getMemoryOperandNo() with X86::getFirstAddrOperandIdx() in the aforementioned mitigation passes.

BTW, I adjust the code by LLVM coding standards.

llvmbot added the backend:X86 label Jan 13, 2024

phoebewang reviewed Jan 13, 2024

View reviewed changes

phoebewang approved these changes Jan 14, 2024

View reviewed changes

phoebewang changed the title ~~[X86] Add MI-layer routine for getting the index of the first address operand~~ [X86] Add MI-layer routine for getting the index of the first address operand. NFC Jan 14, 2024

KanRobert self-requested a review January 15, 2024 03:14

KanRobert reviewed Jan 15, 2024

View reviewed changes

nmosier added 5 commits January 15, 2024 20:35

Address comments

f42e9fa

Revert X86DiscriminateMemOps, X86OptimizeLEAs. Remove extraneous change in X86SpeculativeLoadHardening. Revert vaargs.ll test, since X86OptimizeLEAs was reverted.

const int -> int

1d59d8c

Fall back to MC-layer X86II::getMemoryOperandNo() first

d71f88f

Revert uses of X86II::getMemoryOperandNo() in optimization passes

0c3f686

nmosier force-pushed the x86-memrefs branch from ed44cc6 to 0c3f686 Compare January 15, 2024 21:32

KanRobert reviewed Jan 16, 2024

View reviewed changes

llvm/lib/Target/X86/X86InstrInfo.cpp Show resolved Hide resolved

Expensive checks

e58cc26

KanRobert changed the title ~~[X86] Add MI-layer routine for getting the index of the first address operand. NFC~~ [X86] Add MI-layer routine for getting the index of the first address operand, NFC Jan 16, 2024

KanRobert approved these changes Jan 16, 2024

View reviewed changes

KanRobert merged commit 855e863 into llvm:main Jan 16, 2024
3 of 4 checks passed

KanRobert added a commit that referenced this pull request Jan 16, 2024

[X86] Fix error: unused variable 'isMemOp' after #78019, NFCI

199117a

BTW, I adjust the code by LLVM coding standards.

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024

[X86] Fix error: unused variable 'isMemOp' after llvm#78019, NFCI

dadc30a

BTW, I adjust the code by LLVM coding standards.

nmosier deleted the x86-memrefs branch February 1, 2024 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[X86] Add MI-layer routine for getting the index of the first address operand, NFC #78019

[X86] Add MI-layer routine for getting the index of the first address operand, NFC #78019

nmosier commented Jan 13, 2024 •

edited

llvmbot commented Jan 13, 2024

phoebewang Jan 13, 2024

nmosier Jan 13, 2024

phoebewang Jan 13, 2024

nmosier Jan 13, 2024

phoebewang Jan 13, 2024

nmosier Jan 13, 2024

phoebewang left a comment

phoebewang Jan 14, 2024

nmosier Jan 14, 2024

phoebewang Jan 14, 2024

nmosier Jan 14, 2024

KanRobert commented Jan 15, 2024

phoebewang commented Jan 15, 2024

KanRobert Jan 15, 2024

KanRobert commented Jan 15, 2024

nmosier commented Jan 15, 2024

KanRobert Jan 16, 2024

nmosier Jan 16, 2024

KanRobert Jan 16, 2024

nmosier Jan 16, 2024

KanRobert left a comment

nmosier commented Jan 16, 2024

KanRobert commented Jan 16, 2024

		; RUN: llc -verify-machineinstrs -mcpu=corei7-avx -disable-x86-lea-opt %s -o - \| FileCheck %s --check-prefix=CHECK --check-prefix=NO-FLAGS
		; Disable the x86-optimize-LEAs pass, since it may introduce an intermediate LEA instruction that changes the base register of the vmovaps instructions from %rsp to something else.

		assert(none_of(Desc.operands(), isMemOp) &&
		"Got false negative from X86II::getMemoryOperandNo()!");

		assert(none_of(Desc.operands(), isMemOp) &&
		"Expected no operands to have OPERAND_MEMORY type!");

[X86] Add MI-layer routine for getting the index of the first address operand, NFC #78019

[X86] Add MI-layer routine for getting the index of the first address operand, NFC #78019

Conversation

nmosier commented Jan 13, 2024 • edited

llvmbot commented Jan 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoebewang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KanRobert commented Jan 15, 2024

phoebewang commented Jan 15, 2024

Choose a reason for hiding this comment

KanRobert commented Jan 15, 2024

nmosier commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KanRobert left a comment

Choose a reason for hiding this comment

nmosier commented Jan 16, 2024

KanRobert commented Jan 16, 2024

nmosier commented Jan 13, 2024 •

edited