Page MenuHomePhabricator

[MachineVerifier] Improve checks of target instructions operands.
Needs ReviewPublic

Authored by jonpa on Jun 29 2019, 5:08 AM.

Details

Summary

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?)...

Diff Detail

Event Timeline

jonpa created this revision.Jun 29 2019, 5:08 AM
jonpa edited the summary of this revision. (Show Details)Jun 29 2019, 5:13 AM
arsenm added inline comments.Jun 29 2019, 7:13 AM
lib/CodeGen/MachineVerifier.cpp
1511

Do you really need this check? I would expect this to work naturally with generic instruction operands

test/MachineVerifier/verify-targetops.mir
1 ↗(On Diff #207197)

This doesn't need to run most of the passes, just the verifier with -run-pass=none

3 ↗(On Diff #207197)

This needs a requires x86 target

jonpa updated this revision to Diff 207533.Jul 2 2019, 6:10 AM
jonpa marked 3 inline comments as done.

Thank you for review.

Do you really need this check? I would expect this to work naturally with generic instruction operands

OK, I removed that check and got some new errors:

FAIL: LLVM :: CodeGen/MIR/X86/liveout-register-mask.mir
*** Bad machine code: Expected a non-register operand. ***
- function:    small_patchpoint_codegen
- basic block: %bb.0 entry (0x5d637d8)
- instruction: PATCHPOINT 5, 5, 0, 2, 0, $rdi, $rsi, <regmask $bh $bl $bp $bph $bpl $bx $ebp $ebx $hbp $hbx $rbp $rbx $r12 $r13 $r14 $r15 $r12b $r13b $r14b $r15b $r12bh $r13bh $r14bh $r15bh $r12d $r13d $r14d $r15d $r12w $r13w $r14w $r15w $r12wh and 3 more...>, liveout($esp, $rsp, $sp, $spl), implicit\
-def dead early-clobber $r11, implicit-def $rsp, implicit-def dead $rax
- operand 5:   $rdi

FAIL: LLVM :: CodeGen/PowerPC/ppc64-anyregcc.ll
*** Bad machine code: Expected a non-register operand. ***
- function:    test
- basic block: %bb.0 entry (0x5d8a0e8)
- instruction: PATCHPOINT 0, 40, 0, 2, 13, killed %1:gprc, killed %0:gprc, 2, 3, <regmask $cr0 $cr1 $cr2 $cr3 $cr4 $cr5 $cr6 $cr7 $f0 $f1 $f2 $f3 $f4 $f5 $f6 $f7 $f8 $f9 $f10 $f11 $f12 $f13 $f14 $f15 $f16 $f17 $f18 $f19 $f20 $f21 $f22 $f23 $f24 and 93 more...>, implicit-def dead early-clobber $x12, i\
mplicit-def dead early-clobber $lr8, implicit-def dead early-clobber $ctr8, implicit-def $r1, implicit $x2
- operand 5:   killed %1:gprc
LLVM ERROR: Found 1 machine code errors.

... and more PATCHPOINT fails...

PATCHPOINT has a 'variable_ops' operand, where %1:gprc is the first optional operand. So I added a guard against optional operands, and there were then just two additional test failures compared to before:

FAIL: LLVM :: CodeGen/X86/xray-custom-log.ll
*** Bad machine code: Expected a non-register operand. ***
- function:    fn
- basic block: %bb.0  (0x5da5768)
- instruction: PATCHABLE_EVENT_CALL killed %1:gr64, killed %0:gr32
- operand 1:   killed %0:gr32
LLVM ERROR: Found 1 machine code errors.

PATCHABLE_EVENT_CALL is declared to have an immediate second source operand, where in this case a register operand has been added instead.

FAIL: LLVM :: CodeGen/X86/xray-typed-event-log.ll
*** Bad machine code: Expected a non-register operand. ***
- function:    fn
- basic block: %bb.0  (0x5da5988)
- instruction: PATCHABLE_TYPED_EVENT_CALL killed %2:gr16, killed %1:gr64, killed %0:gr32
- operand 0:   killed %2:gr16

*** Bad machine code: Expected a non-register operand. ***
- function:    fn
- basic block: %bb.0  (0x5da5988)
- instruction: PATCHABLE_TYPED_EVENT_CALL killed %2:gr16, killed %1:gr64, killed %0:gr32
- operand 2:   killed %0:gr32

PATCHABLE_TYPED_EVENT_CALL is supposed to have as the first source operand an immedate per the declaration.

arsenm accepted this revision.Jul 3 2019, 7:53 PM
This revision is now accepted and ready to land.Jul 3 2019, 7:53 PM
jonpa requested review of this revision.Jul 4 2019, 4:08 AM

Thanks for the review of the patch so far!

The tests I listed are still failing, see above for details. What should be done about them? Could perhaps someone from each target look into the problems?

The tests I listed are still failing, see above for details. What should be done about them? Could perhaps someone from each target look into the problems?

MIPS tests failed due a bug. I hope to fix it soon.

arsenm added a comment.Jul 8 2019, 5:26 PM

Thanks for the review of the patch so far!

The tests I listed are still failing, see above for details. What should be done about them? Could perhaps someone from each target look into the problems?

The tests seem broken and just need to be updated

I hope the problem with MIPS tests was fixed at the rL365870.

jonpa added a comment.Jul 16 2019, 6:14 AM

I hope the problem with MIPS tests was fixed at the rL365870.

Yes :-)

I see now:

Failing Tests (5):

LLVM :: CodeGen/Hexagon/expand-condsets-phys-reg.mir
LLVM :: CodeGen/Hexagon/sdr-global.mir
LLVM :: CodeGen/X86/xray-custom-log.ll
LLVM :: CodeGen/X86/xray-typed-event-log.ll
LLVM :: DebugInfo/X86/live-debug-vars-discard-invalid.mir

Should these be "updated"? How?