[MachineVerifier] Improve checks of target instructions operands.

[jonpa]

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

[arsenm]

lib/CodeGen/MachineVerifier.cpp

1511 ↗

(On Diff #207197)

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]

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.

[jonpa]

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]

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]

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]

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

[jonpa]

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]

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]

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

[bjope]

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]

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]

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]

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

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.

[jonpa]

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]

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]

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

[jonpa]

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

[jonpa]

ping!

[jonpa]

ping

[craig.topper]

Is the list from August still the broken test list?

[jonpa]

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]

The two xray tests should be fixed now.

[jonpa]

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]

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]

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

[jonpa]

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

Like this?

[fhahn]

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]

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.

[jonpa]

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]

Fixed the Hexagon failures in 824b25fc02dc4544adae55e6451d355f4c6d7055.

[kparzysz]

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

[jonpa]

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.

[lebedev.ri]

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]

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

[lebedev.ri]

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]

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

[jonpa]

llvm/test/CodeGen/SPARC/fp128.ll

6–7 ↗

(On Diff #231033)

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

[fhahn]

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–1615

nit: no braces needed.

1615

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

nit: no braces needed.

[jonpa]

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]

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]

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]

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]

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

llvm/lib/CodeGen/MachineVerifier.cpp

if (MONum < NumDefs) {
     else if (!MO->isDef() && !MCOI.isOptionalDef())
       report("Explicit definition marked as use", MO, MONum);
     else if (MO->isImplicit())
       report("Explicit definition marked as implicit", MO, MONum);
   } else if (MONum < MCID.getNumOperands()) {
     const MCOperandInfo &MCOI = MCID.OpInfo[MONum];
     // Don't check if it's the last operand in a variadic instruction. See,
     // e.g., LDM_RET in the arm back end.
-    if (MO->isReg() &&
-        !(MI->isVariadic() && MONum == MCID.getNumOperands()-1)) {
+    bool IsOptional = (MI->isVariadic() && MONum == MCID.getNumOperands() - 1);
+    if (MO->isReg() && !IsOptional) {
       if (MO->isDef() && !MCOI.isOptionalDef())
         report("Explicit operand marked as def", MO, MONum);
       if (MO->isImplicit())
         report("Explicit operand marked as implicit", MO, MONum);
     }
 
+    // Check that an instruction has register operands only as expected.
+    if (!IsOptional) {
+      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())
         report("Tied use must be a register", MO, MONum);
       else if (!MO->isTied())
         report("Operand should be tied", MO, MONum);
       else if (unsigned(TiedTo) != MI->findTiedOperandIdx(MONum))
         report("Tied def doesn't match MCInstrDesc", MO, MONum);

llvm/test/CodeGen/Hexagon/expand-condsets-phys-reg.mir

 # RUN: llc -march=hexagon -run-pass expand-condsets %s -o - | FileCheck %s
 # REQUIRES: asserts
 
+# XFAIL: *
+# *** Bad machine code: Expected a register operand. ***
+# - function:    fred
+# - basic block: %bb.0  (0x6325888)
+# - instruction: %1:predregs = C2_cmplt %0:intregs, 10
+# - operand 2:   10
+
 # The physical register as an operand to C2_mux caused a crash.
 # Check that this compiles successfully and that the mux is expanded.
 
 # CHECK: %2:intregs = A2_tfrt %1, $r31
 # CHECK: %2:intregs = A2_tfrf killed %1, killed %0, implicit %2(tied-def 0)
 
 name: fred
 tracksRegLiveness: true

llvm/test/CodeGen/Hexagon/sdr-global.mir

 # RUN: llc -march=hexagon -run-pass hexagon-split-double %s -o - | FileCheck %s
 
+# XFAIL: *
+# *** Bad machine code: Expected a register operand. ***
+# - function:    fred
+# - basic block: %bb.0  (0x6325c88)
+# - instruction: %0:doubleregs = A4_combineir 0, @g0
+# - operand 2:   @g0
+
 # This used to crash because the constant operand was not an immediate.
 # Make sure we can handle such a case.
 
 # CHECK: = A2_tfrsi 0
 # CHECK: = A2_tfrsi @g0
 
 --- |
   define i32 @fred() {

llvm/test/MachineVerifier/verify-regops.mir

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