[PATCH] D63973: [MachineVerifier] Improve checks of target instructions operands.

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa created this revision.
jonpa added reviewers: arsenm, MatzeB, fhahn, kparzysz.
Herald added subscribers: atanasyan, arichardson, wdng, sdardis.

While working with a patch for instruction selection a splitting of a large immediate ended up treated incorrectly by the backend. Where a register operand should have been created, instead it became an immediate. The 3-address instruction was created like

<REG0> = <IMM>, <REG2>,

which is obviously wrong. To my surprise, the machine verifier failed to report this! I don't know why, but I certainly think it should have, so I made a little patch to improve it. While at it, I also added the inverse check for when the MachineOperand is a register, but the descriptor says it should be an immediate.

These tests now fail:

Mips: micromips-sw.ll, tailcall/tailcall.ll:

  - Bad machine code: Expected a register operand. ***
- function:    caller1
- basic block: %bb.0 entry (0x5df9a58)
- instruction: JALRC16_MMR6 @callee1, <regmask $fp $ra $f20 $f22 $f24 $f26 $f28 $f30 $f_hi20 $f_hi22 $f_hi24 $f_hi26 $f_hi28 $f_hi30 $s0 $s1 $s2 $s3 $s4 $s5 $s6 $s7 $d20_64 $d22_64 $d24_64 $d26_64 $d28_64 $d30_64>, implicit-def dead $ra, implicit $a0, implicit $a1, implicit $a2, implicit $a3, implici\

t-def $sp, implicit-def $v0

- operand 0:   @callee1

@callee1 is a MO_GlobalAddress, but OperandType is OPERAND_REGISTER.

Hexagon/sdr-global.mir fails in the same way:

  - Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x5d405d8)
- instruction: %0:doubleregs = A4_combineir 0, @g0
- operand 2:   @g0

Hexagon/expand-condsets-phys-reg.mir fails with an immediate instead of a reg:

  - Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x5d401c8)
- instruction: %1:predregs = C2_cmplt %0:intregs, 10
- operand 2:   10

DebugInfo/X86/live-debug-vars-discard-invalid.mir:

  - Bad machine code: Expected a register operand. ***
- function:    foobar
- basic block: %bb.4  (0x5d755a8)
- instruction: %1:gr64 = BTS64rr %1:gr64(tied-def 0), 0, implicit-def $eflags
- operand 2:   0

The new test case test/MachineVerifier/verify-targetops.mir is a small function with purposefully corrupted operands. In addition to the two errors that are checked for, this is actually also reported:

  - Bad machine code: Tied use must be a register ***
- function:    fun
- basic block: %bb.0  (0x5d7fa88)
- instruction: %1:gr32 = XOR32rm -1, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
- operand 1:   -1

Not sure if that matters (what is a 3-address instruction that would serve well here?)...


https://reviews.llvm.org/D63973

Files:
  lib/CodeGen/MachineVerifier.cpp
  test/MachineVerifier/verify-targetops.mir


Index: test/MachineVerifier/verify-targetops.mir
===================================================================
--- /dev/null
+++ test/MachineVerifier/verify-targetops.mir
@@ -0,0 +1,36 @@
+# RUN: not llc -march=x86 -o - %s -start-after=finalize-isel -verify-machineinstrs \
+# RUN:   2>&1 | FileCheck %s
+#
+# Check that MachineVerifier catches corrupt operands where MO->isReg()
+# returns true, but the descriptor says it should be an OPERAND_IMMEDIATE or
+# OPERAND_PCREL. Conversely, if MO->isReg() (and MO->isFI()) returns false,
+# check that not an OPERAND_REGISTER is expected.
+
+# CHECK-LABEL: fun
+
+# CHECK: *** Bad machine code: Expected a register operand. ***
+# CHECK: - instruction: %1:gr32 = XOR32rm -1, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
+# CHECK: - operand 1:   -1
+
+# CHECK: *** Bad machine code: Expected a non-register operand. ***
+# CHECK: - instruction: %2:gr32 = OR32ri %1:gr32(tied-def 0), %0:gr32, implicit-def dead $eflags
+# CHECK: - operand 2:   %0:gr32
+
+
+name:            fun
+tracksRegLiveness: true
+fixedStack:
+  - { id: 1, offset: 8, size: 4, alignment: 8, isImmutable: true }
+  - { id: 3, size: 4, alignment: 16, isImmutable: true }
+body:             |
+  bb.0:
+    %0:gr32 = MOV32rm %fixed-stack.3, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.3, align 16)
+    ; Was: %1:gr32 = XOR32rm %0, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
+    %1:gr32 = XOR32rm -1, %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $eflags :: (load 4 from %fixed-stack.1, align 8)
+    ; Was: %2:gr32 = OR32ri %1, -256, implicit-def dead $eflags
+    %2:gr32 = OR32ri %1, %0, implicit-def dead $eflags
+    %3:gr32 = MOV32ri -1
+    $eax = COPY %2
+    $edx = COPY %3
+    RET 0, $eax, $edx
+...
Index: lib/CodeGen/MachineVerifier.cpp
===================================================================
--- lib/CodeGen/MachineVerifier.cpp
+++ lib/CodeGen/MachineVerifier.cpp
@@ -1507,6 +1507,16 @@
         report("Explicit operand marked as implicit", MO, MONum);
     }
 
+    // Check that a target instruction has register operands only as expected.
+    if (!TII->isGenericOpcode(MO->getParent()->getOpcode())) {
+      if (MCOI.OperandType == MCOI::OPERAND_REGISTER &&
+          (!MO->isReg() && !MO->isFI()))
+        report("Expected a register operand.", MO, MONum);
+      if ((MCOI.OperandType == MCOI::OPERAND_IMMEDIATE ||
+           MCOI.OperandType == MCOI::OPERAND_PCREL) && MO->isReg())
+        report("Expected a non-register operand.", MO, MONum);
+    }
+
     int TiedTo = MCID.getOperandConstraint(MONum, MCOI::TIED_TO);
     if (TiedTo != -1) {
       if (!MO->isReg())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D63973.207197.patch
Type: text/x-patch
Size: 2770 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190629/0435ebd5/attachment.bin>