This is an archive of the discontinued LLVM Phabricator instance.

[MachineVerifier] Improve checks of target instructions operands.
ClosedPublic

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 ↗(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 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!).

Is the list from August still the broken test list?

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

The two xray tests should be fixed now.

jonpa added a comment.Nov 1 2019, 2:26 AM

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.

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

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.

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

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

jonpa updated this revision to Diff 231033.Nov 26 2019, 2:59 AM

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.

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

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

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.