[X86] Check the address in machine verifier

[skan]

Summary

1. The scale factor must be 1, 2, 4, 8
2. The displacement must fit in 32-bit signed integer

Noticed by: https://github.com/llvm/llvm-project/issues/55091

[fhahn]

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

9

Is the IR here needed? From a quick glance, the only references in the MIR are the basic block names bb and bb2, but those can be removed I think.

[skan]

Remove redundant IR/MIR in the test

[skan]

Updated the test file

1. Removed dependency on opaque pointer
2. Checked there are exactly 2 errors

[skan]

Check the scale only when index register is not none

This change fixed three tests:

LLVM :: CodeGen/X86/optimize-compare.mir
LLVM :: CodeGen/X86/segmented-stacks-dynamic.ll
LLVM :: CodeGen/X86/segmented-stacks.ll

[skan]

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

9

You're right! Done

[pengfei]

LGTM. Thanks!

[pengfei]

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

2

I'm curious why we can verify it without option -verify-machineinstrs?

[skan]

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

2

Because when you feed a MIR to llc w/ option run-pass, the verification for MIR is forced to run.

[skan]

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

2

See https://reviews.llvm.org/rGc7bf20403bee51789f3691f0bf9bc7d7216f3f28 for details.

[craig.topper]

Do we have test coverage that the expensive checks bot runs that would have caught the gather+opaque ptr issue that motivated this patch?

I'm not opposed to this patch, just wondering if it is enough to help us find issues.

[skan]

In D124455#3476213, @craig.topper wrote:

Do we have test coverage that the expensive checks bot runs that would have caught the gather+opaque ptr issue that motivated this patch?

I'm not opposed to this patch, just wondering if it is enough to help us find issues.

I think so. https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/TargetPassConfig.cpp#L809
Machine verify is enabled when the macro EXPENSIVE_CHECKS is on, so we can expose the "gather+opaque ptr" issue at ISEL stage w/ this patch.

[skan]

In D124455#3476229, @skan wrote:

In D124455#3476213, @craig.topper wrote:

Do we have test coverage that the expensive checks bot runs that would have caught the gather+opaque ptr issue that motivated this patch?

I'm not opposed to this patch, just wondering if it is enough to help us find issues.

I think so. https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/TargetPassConfig.cpp#L809
Machine verify is enabled when the macro EXPENSIVE_CHECKS is on, so we can expose the "gather+opaque ptr" issue at ISEL stage w/ this patch.

If the bot has an expensive check for opaque ptr, then we will have the test converage. ( I'm not sure whether bot has such config)

[pengfei]

In D124455#3476213, @craig.topper wrote:

Do we have test coverage that the expensive checks bot runs that would have caught the gather+opaque ptr issue that motivated this patch?

I'm not opposed to this patch, just wondering if it is enough to help us find issues.

The reported issues didn't mention their origin, but I guess they are not from build bot since Nikita's patch merged for almost 20 days.

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

2

Good to know, thanks!

llvm/lib/Target/X86/X86InstrInfo.h

public:
   void buildOutlinedFrame(MachineBasicBlock &MBB, MachineFunction &MF,
                           const outliner::OutlinedFunction &OF) const override;
 
   MachineBasicBlock::iterator
   insertOutlinedCall(Module &M, MachineBasicBlock &MBB,
                      MachineBasicBlock::iterator &It, MachineFunction &MF,
                      outliner::Candidate &C) const override;
 
+  bool verifyInstruction(const MachineInstr &MI,
+                         StringRef &ErrInfo) const override;
 #define GET_INSTRINFO_HELPER_DECLS
 #include "X86GenInstrInfo.inc"
 
   static bool hasLockPrefix(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & X86II::LOCK;
   }
 
   Optional<ParamLoadedValue> describeLoadedValue(const MachineInstr &MI,

llvm/lib/Target/X86/X86InstrInfo.cpp

X86InstrInfo::getAddrModeFromMemoryOp(const MachineInstr &MemI,
   ExtAddrMode AM;
   AM.BaseReg = BaseOp.getReg();
   AM.ScaledReg = MemI.getOperand(MemRefBegin + X86::AddrIndexReg).getReg();
   AM.Scale = MemI.getOperand(MemRefBegin + X86::AddrScaleAmt).getImm();
   AM.Displacement = DispMO.getImm();
   return AM;
 }
 
+bool X86InstrInfo::verifyInstruction(const MachineInstr &MI,
+                                     StringRef &ErrInfo) const {
+  Optional<ExtAddrMode> AMOrNone = getAddrModeFromMemoryOp(MI, nullptr);
+  if (!AMOrNone)
+    return true;
+
+  ExtAddrMode AM = *AMOrNone;
+
+  if (AM.ScaledReg != X86::NoRegister) {
+    switch (AM.Scale) {
+    case 1:
+    case 2:
+    case 4:
+    case 8:
+      break;
+    default:
+      ErrInfo = "Scale factor in address must be 1, 2, 4 or 8";
+      return false;
+    }
+  }
+  if (!isInt<32>(AM.Displacement)) {
+    ErrInfo = "Displacement in address must fit into 32-bit signed "
+              "integer";
+    return false;
+  }
+
+  return true;
+}
+
 bool X86InstrInfo::getConstValDefinedInReg(const MachineInstr &MI,
                                            const Register Reg,
                                            int64_t &ImmVal) const {
   if (MI.getOpcode() != X86::MOV32ri && MI.getOpcode() != X86::MOV64ri)
     return false;
   // Mov Src can be a global address.
   if (!MI.getOperand(1).isImm() || MI.getOperand(0).getReg() != Reg)
     return false;

llvm/test/CodeGen/MIR/X86/machine-verifier-address.mir

+# RUN: not --crash llc -march=x86-64 -run-pass none -o /dev/null %s 2>&1 | FileCheck %s
+# This test ensures that the address is checked in machine verifier.
+
+---
+name:            baz
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1(0x80000000)
+    liveins: $rdi, $xmm0
+  
+    %1:vr128 = COPY $xmm0
+    %0:gr64 = COPY $rdi
+    %2:vr128 = COPY %1
+  
+  bb.1:
+    successors: %bb.1(0x80000000)
+  
+    %3:vr256 = AVX_SET0
+    %4:vr128 = VPSLLDri %2, 31
+    %5:vr256 = VPMOVSXDQYrr killed %4
+    %8:vr256 = IMPLICIT_DEF
+    ; CHECK: *** Bad machine code: Scale factor in address must be 1, 2, 4 or 8 ***
+    %6:vr256, %7:vr256 = VGATHERQPDYrm %3, %0, 16, killed %8, 0, $noreg, %5 :: (load unknown-size, align 8)
+    %9:vr128 = COPY %6.sub_xmm
+    ; CHECK: *** Bad machine code: Displacement in address must fit into 32-bit signed integer ***
+    VMOVLPDmr $noreg, 1, $noreg, 1111111111111, $noreg, killed %9 :: (store (s64) into `i64* undef`)
+    JMP_1 %bb.1
+    ; CHECK: LLVM ERROR: Found 2 machine code errors
+
+...