This is an archive of the discontinued LLVM Phabricator instance.

Differential D63973

[MachineVerifier] Improve checks of target instructions operands.
ClosedPublic

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

Details

Reviewers
arsenm
MatzeB
fhahn
kparzysz
craig.topper
dberris
bjope
thegameg
aemerson
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
Herald added subscribers: atanasyan, arichardson, wdng, sdardis. · View Herald TranscriptJun 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?

atanasyan added a comment.Jul 8 2019, 4:30 AM
In D63973#1570197, @jonpa wrote:

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
In D63973#1570197, @jonpa wrote:

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

atanasyan added a comment.Jul 11 2019, 10:25 PM

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

jonpa added a comment.Jul 16 2019, 6:14 AM
In D63973#1582205, @atanasyan wrote:

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?

jonpa added a reviewer: craig.topper.Aug 25 2019, 1:46 AM

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 reviewers: dberris, bjope, thegameg.Aug 25 2019, 1:58 AM
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
In D63973#1644277, @lebedev.ri wrote:

@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
In D63973#1644279, @bjope wrote:
In D63973#1644277, @lebedev.ri wrote:

@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
In D63973#1644283, @bjope wrote:
In D63973#1644279, @bjope wrote:
In D63973#1644277, @lebedev.ri wrote:

@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...

lebedev.ri added a comment.Aug 25 2019, 11:54 AM

@kparzysz might be able to help with hexagon failures.

In D63973#1644274, @jonpa wrote:

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
In D63973#1645986, @thegameg wrote:

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

dberris added a comment.Aug 27 2019, 1:44 AM
In D63973#1645986, @thegameg wrote:

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.

jonpa added a comment.Sep 16 2019, 12:44 AM

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

jonpa added a comment.Sep 30 2019, 12:41 AM

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

jonpa added a comment.Oct 8 2019, 1:52 AM

ping!

jonpa added a comment.Oct 22 2019, 1:55 AM

ping

craig.topper added a comment.Oct 23 2019, 9:06 PM

Is the list from August still the broken test list?

jonpa added a reviewer: aemerson.Oct 24 2019, 2:09 AM
In D63973#1719410, @craig.topper wrote:

Is the list from August still the broken test list?

The Hexagon and X86 CodeGen test failures are still the same but the DebugInfo failure does not show up anymore. A new failure is in AArch64/GlobalISel:

******************** TEST 'LLVM :: CodeGen/AArch64/GlobalISel/legalize-phi-insertpt-decrement.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    snork
- basic block: %bb.3 bb10 (0x62e5ce8)
- instruction: RET 0
- operand 0:   0
LLVM ERROR: Found 1 machine code errors.

******************** TEST 'LLVM :: CodeGen/Hexagon/expand-condsets-phys-reg.mir' FAILED ********************
*** Bad machine code: Expected a register operand. ***
- function:    fred
- basic block: %bb.0  (0x62c1c18)
- 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  (0x62c2028)
- instruction: %0:doubleregs = A4_combineir 0, @g0
- operand 2:   @g0
LLVM ERROR: Found 1 machine code errors.

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

******************** TEST 'LLVM :: CodeGen/X86/xray-typed-event-log.ll' FAILED ********************
*** Bad machine code: Expected a non-register operand. ***
- function:    fn
- basic block: %bb.0  (0x632a8b8)
- 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  (0x632a8b8)
- 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.

Failing Tests (5):
    LLVM :: CodeGen/AArch64/GlobalISel/legalize-phi-insertpt-decrement.mir
    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
craig.topper added a comment.Oct 30 2019, 11:05 AM

The two xray tests should be fixed now.

jonpa added a comment.Nov 1 2019, 2:26 AM
In D63973#1727465, @craig.topper wrote:

The two xray tests should be fixed now.

Yes, thank you.

A "ping" to Hexagon and AArch64 folks to fix the broken tests as listed previously.

jonpa added a comment.Nov 18 2019, 1:27 AM

Ping!

Still waiting for Hexagon (2 tests) and AArch64 (1 test) maintainers to fix the corrupted tests as listed previously. This is an improvement to the machine-verifier that has detected existing tests with broken instruction operands. These are .mir tests so I would expect this to not be that much work, but still best be done by someone working on the particular target.

arsenm added a comment.Nov 18 2019, 1:36 AM

I'm inclined to xfail those tests and open bugs for them

jonpa updated this revision to Diff 230025.Nov 19 2019, 3:21 AM

I'm inclined to xfail those tests and open bugs for them

Like this?

Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2019, 3:21 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn added a comment.Nov 19 2019, 12:34 PM
In D63973#1751315, @jonpa wrote:

I'm inclined to xfail those tests and open bugs for them

Like this?

That approach sounds good to me! It would be great if you could file issues for the XFAIL'd tests though, so we can make sure the right people are aware and we do not lose track.

aemerson added a comment.Nov 19 2019, 1:42 PM
In D63973#1751315, @jonpa wrote:

I'm inclined to xfail those tests and open bugs for them

Like this?

Sorry, I fixed the AArch64 test yesterday and didn't get a chance to commit it, it's now in cf823ce4ad9.

aemerson mentioned this in rGcf823ce4ad9d: [AArch64] Fix MIR test instruction to not have invalid operand..Nov 19 2019, 1:47 PM
jonpa mentioned this in D70501: [SystemZ] Don't build a PPA instruction with an immediate 0.Nov 20 2019, 10:58 AM
jonpa updated this revision to Diff 230299.Nov 20 2019, 11:28 AM

XFAIL of AArch64 removed since it was fixed - thank you!

Sorry, I now see that with expensive checks enabled three more tests fail:

LLVM :: CodeGen/Hexagon/vasrh.select.ll
LLVM :: CodeGen/SPARC/fp128.ll
LLVM :: CodeGen/SystemZ/htm-intrinsics.ll

CodeGen/Hexagon/vasrh.select.ll:

# After Instruction Selection
*** Bad machine code: Expected a register operand. ***
- function:    f0
- basic block: %bb.0 b0 (0x639e558)
- instruction: %1:doubleregs = S2_asr_r_vh killed %0:doubleregs, 62
- operand 2:   62
LLVM ERROR: Found 1 machine code errors.

test/CodeGen/SPARC/fp128.ll

# After Instruction Selection
*** Bad machine code: Expected a register operand. ***
- function:    f128_compare
- basic block: %bb.0 entry (0x63ed058)
- instruction: CMPrr killed %21:intregs, 0, implicit-def $icc
- operand 1:   0
LLVM ERROR: Found 1 machine code errors.

I have proposed a patch for the SystemZ test case at https://reviews.llvm.org/D70501.

Hexagon test failures (all three) reported at https://bugs.llvm.org/show_bug.cgi?id=44090
SPARC failure reported at https://bugs.llvm.org/show_bug.cgi?id=44091

Would it be ok to XFAIL also the test cases that fail only with expensive checks enabled and then commit?

kparzysz added a comment.Nov 20 2019, 1:11 PM

Fixed the Hexagon failures in 824b25fc02dc4544adae55e6451d355f4c6d7055.

kparzysz added a comment.Nov 21 2019, 7:40 AM

And the vasrh one (that I missed) in e8d1578131247d089209952476ba9191ad0295be. Hexagon tests should now pass.

jonpa mentioned this in rG3ec193fb527e: [SystemZ] Don't build a PPA instruction with an immediate 0 operand..Nov 26 2019, 2:25 AM
jonpa updated this revision to Diff 231033.Nov 26 2019, 2:59 AM
In D63973#1754038, @kparzysz wrote:

Fixed the Hexagon failures in 824b25fc02dc4544adae55e6451d355f4c6d7055.

Thank you!

SystemZ test case fixed by 3ec193f. This means that the only remaining failing test case is SPARC/fp128.ll, which fails with expensive checks enabled, and I have XFAIL:ed it.

Herald added subscribers: fedor.sergeev, jyknight. · View Herald TranscriptNov 26 2019, 2:59 AM
lebedev.ri added a comment.Nov 26 2019, 3:19 AM
In D63973#1759970, @jonpa wrote:
In D63973#1754038, @kparzysz wrote:

Fixed the Hexagon failures in 824b25fc02dc4544adae55e6451d355f4c6d7055.

Thank you!

SystemZ test case fixed by 3ec193f. This means that the only remaining failing test case is SPARC/fp128.ll, which fails with expensive checks enabled, and I have XFAIL:ed it.

I think we should get this in as-is, the existing failure can be addressed later and we won't risk having new failure creep in.

jonpa added a comment.Nov 26 2019, 4:16 AM

I agree... Let's hope one of the reviewers accept it (unless you want to do it..?).

lebedev.ri added inline comments.Nov 26 2019, 4:31 AM
llvm/test/CodeGen/SPARC/fp128.ll
6–7 ↗(On Diff #231033)

Did you file a bug for this one?
Might be a good idea, and add a link to patch description.

jonpa updated this revision to Diff 231047.Nov 26 2019, 4:46 AM

Added link to Bugzilla in the SPARC XFAIL:ing test case.

jonpa marked 2 inline comments as done.Nov 26 2019, 4:46 AM
jonpa added inline comments.
llvm/test/CodeGen/SPARC/fp128.ll
6–7 ↗(On Diff #231033)

Yes, and now I also added a link to Bugzilla here...

fhahn accepted this revision.Nov 28 2019, 8:55 AM

LGTM, with a few small suggestions. Thanks for your patience. Please wait with committing until early next week, in case people on Thanksgiving break have additional comments.

llvm/lib/CodeGen/MachineVerifier.cpp
1614 ↗(On Diff #231047)

nit: no braces needed.

1615 ↗(On Diff #231047)

Personally, I think it would be slightly better to guard the both blocks with a single if (!Optional) and a comment like ’// Check non-variadic operands only. as both blocks apply to non-variadic operands.

1625 ↗(On Diff #231047)

nit: no braces needed.

This revision is now accepted and ready to land.Nov 28 2019, 8:55 AM
jonpa closed this revision.Dec 3 2019, 1:33 AM
jonpa marked 4 inline comments as done.

Thanks for review! Committed as 4fd8f11.

@fhahn: I tried to follow your suggestions and added your comment to the existing one - please take a look...
(I also changed comment character from # to ; in SPARC test case, which seemed to work better)

fhahn added a comment.Dec 3 2019, 1:43 AM
In D63973#1766594, @jonpa wrote:

Thanks for review! Committed as 4fd8f11.

@fhahn: I tried to follow your suggestions and added your comment to the existing one - please take a look...
(I also changed comment character from # to ; in SPARC test case, which seemed to work better)

Looks great, thanks!

jonpa added a comment.Dec 3 2019, 1:54 AM

It looks like the build bots didn't like this after all - the SPARC XFAIL doesn't fail without the expensive checks, but it is now marked XFAIL to pass with expensive checks enabled...

Is there any way around this? Maybe I could just run the verifier on that test always for now?

Unexpected Passing Tests (1):
    LLVM :: CodeGen/SPARC/fp128.ll
jonpa added a comment.Dec 3 2019, 2:24 AM

The compile time did not increase by running the machine verifier in one of the runs in the SPARC test case, so I added this with 7b63e27, to hopefully get the bots green.

Jim added a subscriber: Jim.Feb 26 2020, 9:35 PM

Fix sparc bug in https://reviews.llvm.org/D75227

Revision Contents

PathSize
lib/
CodeGen/
14 lines
test/
MachineVerifier/
37 lines

Diff 207533

lib/CodeGen/MachineVerifier.cpp

Show First 20 Lines • Show All 1,493 Lines • ▼ Show 20 Lines if (MONum < NumDefs) {
else if (!MO->isDef() && !MCOI.isOptionalDef()) else if (!MO->isDef() && !MCOI.isOptionalDef())
report("Explicit definition marked as use", MO, MONum); report("Explicit definition marked as use", MO, MONum);
else if (MO->isImplicit()) else if (MO->isImplicit())
report("Explicit definition marked as implicit", MO, MONum); report("Explicit definition marked as implicit", MO, MONum);
} else if (MONum < MCID.getNumOperands()) { } else if (MONum < MCID.getNumOperands()) {
const MCOperandInfo &MCOI = MCID.OpInfo[MONum]; const MCOperandInfo &MCOI = MCID.OpInfo[MONum];
// Don't check if it's the last operand in a variadic instruction. See, // Don't check if it's the last operand in a variadic instruction. See,
// e.g., LDM_RET in the arm back end. // e.g., LDM_RET in the arm back end.
if (MO->isReg() && bool IsOptional = (MI->isVariadic() && MONum == MCID.getNumOperands() - 1);
!(MI->isVariadic() && MONum == MCID.getNumOperands()-1)) { if (MO->isReg() && !IsOptional) {
if (MO->isDef() && !MCOI.isOptionalDef()) if (MO->isDef() && !MCOI.isOptionalDef())
report("Explicit operand marked as def", MO, MONum); report("Explicit operand marked as def", MO, MONum);
if (MO->isImplicit()) if (MO->isImplicit())
report("Explicit operand marked as implicit", MO, MONum); report("Explicit operand marked as implicit", MO, MONum);
} }
// Check that an instruction has register operands only as expected.
if (!IsOptional) {
arsenmUnsubmitted
Done
ReplyInline Actions

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

arsenm: Do you really need this check? I would expect this to work naturally with generic instruction…
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); int TiedTo = MCID.getOperandConstraint(MONum, MCOI::TIED_TO);
if (TiedTo != -1) { if (TiedTo != -1) {
if (!MO->isReg()) if (!MO->isReg())
report("Tied use must be a register", MO, MONum); report("Tied use must be a register", MO, MONum);
else if (!MO->isTied()) else if (!MO->isTied())
report("Operand should be tied", MO, MONum); report("Operand should be tied", MO, MONum);
else if (unsigned(TiedTo) != MI->findTiedOperandIdx(MONum)) else if (unsigned(TiedTo) != MI->findTiedOperandIdx(MONum))
report("Tied def doesn't match MCInstrDesc", MO, MONum); report("Tied def doesn't match MCInstrDesc", MO, MONum);
▲ Show 20 Lines • Show All 1,214 Lines • Show Last 20 Lines

test/MachineVerifier/verify-regops.mir

  • This file was added.
# RUN: not llc -march=x86 -o - %s -run-pass=none -verify-machineinstrs \
# RUN: 2>&1 | FileCheck %s
# REQUIRES: x86-registered-target
#
# 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
...