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?

ping!

It would be very nice if people with knowledge of the Hexagon and X86 backends could help out on fixing the failing tests. Note that these tests have been discovered to have a broken MIR (!), so please help...

Here is a condensed output of 'ninja check' with this patch applied:

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

******************** TEST 'LLVM :: CodeGen/Hexagon/expand-condsets-phys-reg.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x6071598)
- instruction: %1:predregs = C2_cmplt %0:intregs, 10
- operand 2:   10
LLVM ERROR: Found 1 machine code errors.

******************** TEST 'LLVM :: CodeGen/Hexagon/sdr-global.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x6071938)
- instruction: %0:doubleregs = A4_combineir 0, @g0
- operand 2:   @g0
LLVM ERROR: Found 1 machine code errors.
FileCheck error: '-' is empty.
FileCheck command line:  /home/ijonpan/llvm/build/llvm-dev/bin/FileCheck /home/ijonpan/llvm/llvm-dev/test/CodeGen/Hexagon/sdr-global.mir

******************** TEST 'LLVM :: CodeGen/X86/xray-custom-log.ll' FAILED ********************
*** Bad machine code: Expected a non-register operand. ***
- function:    fn
- basic block: %bb.0  (0x60d88b8)
- instruction: PATCHABLE_EVENT_CALL killed %1:gr64, killed %0:gr32
- operand 1:   killed %0:gr32
LLVM ERROR: Found 1 machine code errors.
FileCheck error: '-' is empty.
FileCheck command line:  /home/ijonpan/llvm/build/llvm-dev/bin/FileCheck /home/ijonpan/llvm/llvm-dev/test/CodeGen/X86/xray-custom-log.ll

******************** TEST 'LLVM :: CodeGen/X86/xray-typed-event-log.ll' FAILED ********************
*** Bad machine code: Expected a non-register operand. ***
- function:    fn
- basic block: %bb.0  (0x60d8ad8)
- 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  (0x60d8ad8)
- instruction: PATCHABLE_TYPED_EVENT_CALL killed %2:gr16, killed %1:gr64, killed %0:gr32
- operand 2:   killed %0:gr32
LLVM ERROR: Found 2 machine code errors.

******************** TEST 'LLVM :: DebugInfo/X86/live-debug-vars-discard-invalid.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    foobar
- basic block: %bb.4  (0x60a8128)
- instruction: %1:gr64 = BTS64rr %1:gr64(tied-def 0), 0, implicit-def $eflags
- operand 2:   0

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

*** Bad machine code: Expected a register operand. ***
- function:    foobar
- basic block: %bb.4  (0x60a8128)
- instruction: %1:gr64 = BTS64rr %1:gr64(tied-def 0), 0, implicit-def $eflags
- operand 2:   0
LLVM ERROR: Found 3 machine code errors.
lebedev.ri added subscribers: bjope, dberris, lebedev.ri.

@dberris might know about XRay test failures.
@bjope might know about DebugInfo/X86/live-debug-vars-discard-invalid.mir failure

bjope added a comment.Aug 25 2019, 3:10 AM

@bjope might know about DebugInfo/X86/live-debug-vars-discard-invalid.mir failure

Thanks! (I had not noticed this patch)
I think the problem with DebugInfo/X86/live-debug-vars-discard-invalid.mir is that the test case uses the BTS64rr instruction, but it should probably have used BTS64ri8 instead (considering that the second operand is a constant).
I'll have another look at it and then I'll push a fixup.

bjope added a comment.Aug 25 2019, 3:56 AM

@bjope might know about DebugInfo/X86/live-debug-vars-discard-invalid.mir failure

Thanks! (I had not noticed this patch)
I think the problem with DebugInfo/X86/live-debug-vars-discard-invalid.mir is that the test case uses the BTS64rr instruction, but it should probably have used BTS64ri8 instead (considering that the second operand is a constant).
I'll have another look at it and then I'll push a fixup.

Fix pushed in https://reviews.llvm.org/rL369866

jonpa added a comment.Aug 25 2019, 7:58 AM

@bjope might know about DebugInfo/X86/live-debug-vars-discard-invalid.mir failure

Thanks! (I had not noticed this patch)
I think the problem with DebugInfo/X86/live-debug-vars-discard-invalid.mir is that the test case uses the BTS64rr instruction, but it should probably have used BTS64ri8 instead (considering that the second operand is a constant).
I'll have another look at it and then I'll push a fixup.

Fix pushed in https://reviews.llvm.org/rL369866

Thanks for fixing DebugInfo/X86/live-debug-vars-discard-invalid.mir! 4 more to go...

@kparzysz might be able to help with hexagon failures.

ping!

It would be very nice if people with knowledge of the Hexagon and X86 backends could help out on fixing the failing tests. Note that these tests have been discovered to have a broken MIR (!), so please help...

Here is a condensed output of 'ninja check' with this patch applied:

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

******************** TEST 'LLVM :: CodeGen/Hexagon/expand-condsets-phys-reg.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x6071598)
- instruction: %1:predregs = C2_cmplt %0:intregs, 10
- operand 2:   10
LLVM ERROR: Found 1 machine code errors.

******************** TEST 'LLVM :: CodeGen/Hexagon/sdr-global.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x6071938)
- instruction: %0:doubleregs = A4_combineir 0, @g0
- operand 2:   @g0
LLVM ERROR: Found 1 machine code errors.
FileCheck error: '-' is empty.
FileCheck command line:  /home/ijonpan/llvm/build/llvm-dev/bin/FileCheck /home/ijonpan/llvm/llvm-dev/test/CodeGen/Hexagon/sdr-global.mir
thegameg accepted this revision.Aug 26 2019, 3:20 PM

Thanks, this LGTM. I quickly looked at the X86 tests, it seems that PATCHABLE_EVENT_CALL and PATCHABLE_TYPED_EVENT_CALL are always created with a register but expect immediates in Target.td. I'm not sure what was the intention so I'll leave it to @dberris.

This revision is now accepted and ready to land.Aug 26 2019, 3:20 PM
jonpa requested review of this revision.Aug 27 2019, 1:31 AM

Thanks, this LGTM. I quickly looked at the X86 tests, it seems that PATCHABLE_EVENT_CALL and PATCHABLE_TYPED_EVENT_CALL are always created with a register but expect immediates in Target.td. I'm not sure what was the intention so I'll leave it to @dberris.

Thanks for review thegameg! I am changing the status back to "Needs review", though, to avoid confusion...

Thanks, this LGTM. I quickly looked at the X86 tests, it seems that PATCHABLE_EVENT_CALL and PATCHABLE_TYPED_EVENT_CALL are always created with a register but expect immediates in Target.td. I'm not sure what was the intention so I'll leave it to @dberris.

The intention is to have the arguments always be in a specific set of registers because the trampolines for XRay expect those to be in specific registers. I wasn't sure how to achieve this consistently and correctly in the lowering. Any pointers here would be really helpful to me too as I have very limited understanding of the MIR validation to achieve what I was hoping for.

Ping to code owners of the corrupt tests: @kparzysz and @craig.topper

ping! Please fix test cases that have been found broken (on trunk without this patch!).