LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 33047 - SystemZ check has a bunch of machine verifier errors
Summary: SystemZ check has a bunch of machine verifier errors
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: SystemZ (show other bugs)
Version: trunk
Hardware: PC Windows NT
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks: 32146
  Show dependency tree
 
Reported: 2017-05-15 11:37 PDT by Simon Pilgrim
Modified: 2017-06-23 07:33 PDT (History)
8 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pilgrim 2017-05-15 11:37:20 PDT
Enabling machine code verification with EXPENSIVE_CHECKS (https://reviews.llvm.org/D30625) causes a number of machine verifier errors with `ninja check-codegen-systemz`:

Failing Tests (6):
    LLVM :: CodeGen/SystemZ/stack-guard.ll
    LLVM :: CodeGen/SystemZ/trap-01.ll
    LLVM :: CodeGen/SystemZ/trap-02.ll
    LLVM :: CodeGen/SystemZ/trap-03.ll
    LLVM :: CodeGen/SystemZ/trap-04.ll
    LLVM :: CodeGen/SystemZ/trap-05.ll
Comment 1 Jonas Paulsson 2017-05-19 02:10:53 PDT
The trap problem seems to relate to an issue that keeps the SystemZ backend to use the isTerminator flag on the SystemZ::Trap instruction, per the comment in SystemZInstrInfo.td:

// Unconditional trap.
// FIXME: This trap instruction should be marked as isTerminator, but there is
// currently a general bug that allows non-terminators to be placed between
// terminators. Temporarily leave this unmarked until the bug is fixed.
let isTerminator=1, hasCtrlDep = 1, isBarrier = 1 in
  def Trap : Alias<4, (outs), (ins), [(trap)]>;

If I set this flag, I get:

*** IR Dump After Module Verifier ***
define i32 @f0() {
entry:
  tail call void @llvm.trap()
  ret i32 0
}

# After Instruction Selection
# Machine code for function f0: IsSSA, TracksLiveness

BB#0: derived from LLVM BB %entry
        Trap
        %vreg0<def> = LHI 0; GR32Bit:%vreg0
        %R2L<def> = COPY %vreg0; GR32Bit:%vreg0
        Return %R2L<imp-use>

# End machine code for function f0.

*** Bad machine code: Non-terminator instruction after the first terminator ***
- function:    f0
- basic block: BB#0 entry (0x51828c8)
- instruction: %vreg0<def> = LHI
First terminator was:   Trap

Without it, the test fails like:

# After Instruction Selection
# Machine code for function f1: IsSSA, TracksLiveness
Function Live Ins: %R2D in %vreg0

BB#0: derived from LLVM BB %entry
    Live Ins: %R2D
	%vreg0<def> = COPY %R2D; GR64Bit:%vreg0
	%vreg1<def> = COPY %vreg0:subreg_l32; GR32Bit:%vreg1 GR64Bit:%vreg0
	CHI %vreg1<kill>, 15, %CC<imp-def>; GR32Bit:%vreg1
	BRC 14, 4, <BB#2>, %CC<imp-use>
	J <BB#1>
    Successors according to CFG: BB#1(0x00000800 / 0x80000000 = 0.00%) BB#2(0x7ffff800 / 0x80000000 = 100.00%)

BB#1: derived from LLVM BB %if.then
    Predecessors according to CFG: BB#0
	Trap

BB#2: derived from LLVM BB %if.end
    Predecessors according to CFG: BB#0
	%vreg2<def> = LHI 0; GR32Bit:%vreg2
	%R2L<def> = COPY %vreg2; GR32Bit:%vreg2
	Return %R2L<imp-use>

# End machine code for function f1.

*** Bad machine code: MBB exits via unconditional fall-through but ends with a barrier instruction! ***
- function:    f1
- basic block: BB#1 if.then (0x4ee2298)
LLVM ERROR: Found 1 machine code errors.

Does anybody know more about this problem?
Comment 2 Simon Pilgrim 2017-05-19 02:48:57 PDT
Adding people that might be able to help
Comment 3 Jonas Paulsson 2017-05-24 06:14:23 PDT
CodeGen/SystemZ/stack-guard.ll was fixed with r303743.

Still awaiting help on the Trap tests...
Comment 4 Matthias Braun 2017-05-31 11:47:08 PDT
r304320 enabled the machine verifier by default with EXPENSIVE_CHECKS. SystemZ is one of the targets excluded from this by overriding TargetMachine::isMachineVerifierClean() to return false.

Please remove the override when the target at least passes all lit tests without machine verifier failure.
Comment 5 Jonas Paulsson 2017-06-07 06:50:11 PDT
entry:
  tail call void @llvm.trap()
  ret i32 0

is lowered to

BB#0: derived from LLVM BB %entry
        Trap
        %vreg0<def> = LHI 0; GR32Bit:%vreg0
        %R2L<def> = COPY %vreg0; GR32Bit:%vreg0
        Return %R2L<imp-use>

, which is incorrect since the Trap instruction is a terminator. This could be fixed in SystemZTargetLowering::LowerReturn(), so that the Trap is scheduled per its terminator flag.

It seems however that the return shouldn't be there in the first place, since it is unreachable. Could it be that a return (or any other instruction) after a llvm.trap() call should be removed as dead code even before isel?
Comment 6 Jonas Paulsson 2017-06-13 05:57:46 PDT
https://reviews.llvm.org/D34143#58aa05e8
Comment 7 Jonas Paulsson 2017-06-22 05:19:20 PDT
The trap problem seems to be resolved by simply removing barrier and terminator flags on the trap instructions.

What's left now before SystemZ is clean are two (new) fails in CodeGen/Generic, which I am a bit puzzled with:

test/CodeGen/Generic/llc-start-stop.ll:5:20: error: STOP-AFTER-NEXT: is not on the line after the previous match

llc generates:

      Loop Pass Manager
        Induction Variable Users
        Loop Strength Reduction
      Verify generated machine code    <<< unexpected
      MIR Printing Pass

?? This seems normal with expensive checks.

test/CodeGen/Generic/print-machineinstrs.ll:6:10: error: expected string not found in input
; CHECK: -branch-folder -machineinstr-printer
         ^
<stdin>:1:1: note: scanning from here
Pass Arguments: -targetlibinfo -targetpassconfig -machinemoduleinfo -tti -tbaa -scoped-noalias -assumption-cache-tracker -collector-metadata -profile-summary-info -machine-branch-prob -pre-isel-intrinsic-lowering -systemz-tdc -domtree -basicaa -verify -loops -loop-simplify -scalar-evolution -iv-users -loop-reduce -gc-lowering -shadow-stack-gc-lowering -unreachableblockelim -domtree -consthoist -partially-inline-libcalls -cfinserter -scalarize-masked-mem-intrin -expand-reductions -domtree -loops -codegenprepare -rewrite-symbols -domtree -dwarfehprepare -safe-stack -stack-protector -verify -domtree -basicaa -aa -loops -branch-prob -machinedomtree -machineverifier -expand-isel-pseudos -machineverifier -tailduplication -machineverifier -opt-phis -slotindexes -stack-coloring -localstackalloc -dead-mi-elimination -machineverifier -machinedomtree -machine-loops -machine-trace-metrics -early-ifcvt -machineverifier -machinelicm -machine-cse -machinepostdomtree -branch-coalescing -machineverifier -machinedomtree -machinepostdomtree -machine-loops -machine-block-freq -machine-sink -machineverifier -peephole-opt -machineverifier -dead-mi-elimination -machineverifier -detect-dead-lanes -processimpdefs -unreachable-mbb-elimination -livevars -phi-node-elimination -twoaddressinstruction -slotindexes -liveintervals -simple-register-coalescing -machineverifier -rename-independent-subregs -machineverifier -machine-scheduler -machineverifier -machine-block-freq -livedebugvars -livestacks -virtregmap -liveregmatrix -edge-bundles -spill-code-placement -lazy-machine-block-freq -machine-opt-remark-emitter -greedy -machineverifier -virtregrewriter -machineverifier -stack-slot-coloring -machineverifier -machinelicm -machineverifier -machine-block-freq -machinepostdomtree -shrink-wrap -machineverifier -prologepilog -machineverifier -branch-folder -machineverifier -machineinstr-printer -machineverifier -tailduplication -machineverifier -machine-cp -machineverifier -postrapseudos -machineverifier -systemz-expand-pseudo -machineverifier -machinedomtree -machine-loops -machine-block-freq -if-converter -machineverifier -gc-analysis -machinedomtree -machine-loops -machine-block-freq -machinepostdomtree -block-placement -machineverifier -machineverifier -machinedomtree -machine-loops -postmisched -machineverifier -funclet-layout -stackmap-liveness -livedebugvalues -fentry-insert -machinedomtree -machine-loops -xray-instrumentation -patchable-function -lazy-machine-block-freq -machine-opt-remark-emitter -machinedomtree -machine-loops
^

Note that both -branch-folder and -machineinstr-printer are part of that long list.

How could these two test cases fail on SystemZ but not on other targets?
Comment 8 Jonas Paulsson 2017-06-22 05:25:21 PDT
(In reply to Matthias Braun from comment #4)
> r304320 enabled the machine verifier by default with EXPENSIVE_CHECKS.
> SystemZ is one of the targets excluded from this by overriding
> TargetMachine::isMachineVerifierClean() to return false.
> 
> Please remove the override when the target at least passes all lit tests
> without machine verifier failure.

Per my previous comment, ninja check with expensive checks now fails now just twice in CodeGen/Generic.

Is the intent that the target should return true (stop overriding) in isMachineVerifierClean() when the *target* codegen tests passes (check-llvm-codegen-systemz)?
Comment 9 Matthias Braun 2017-06-22 09:17:28 PDT
> Per my previous comment, ninja check with expensive checks now fails now just twice in CodeGen/Generic.

Great!

I think those tests in generic are not generic enough. Sure they test generic facilities like --start-after etc. but the actual output can obviously differ depending on how the target sets up their pass pipeline.
I think we should simply restrict thise two tests to x86.
Comment 10 Jonas Paulsson 2017-06-23 00:06:36 PDT
(In reply to Matthias Braun from comment #9)
> > Per my previous comment, ninja check with expensive checks now fails now just twice in CodeGen/Generic.
> 
> Great!
> 
> I think those tests in generic are not generic enough. Sure they test
> generic facilities like --start-after etc. but the actual output can
> obviously differ depending on how the target sets up their pass pipeline.
> I think we should simply restrict thise two tests to x86.

I would expect those tests to fail also for X86 whith expensive checks, since they simply fail when the verifier is all of a sudden present in the argument / pass list.

Is it possible to disable these tests when EXPENSIVE_CHECKS is defined? 

Otherwise, one idea is to use -verify-machineinstrs on those tests so that it is possible to always expect the verifier there. If you think this is ok, could I go ahead and commit, or should I open a review?
Comment 11 Simon Pilgrim 2017-06-23 04:13:14 PDT
(In reply to Jonas Paulsson from comment #10)
> Otherwise, one idea is to use -verify-machineinstrs on those tests so that
> it is possible to always expect the verifier there. If you think this is ok,
> could I go ahead and commit, or should I open a review?

This makes sense to me, especially if it means it can stay Generic and be tested on all targets. Go ahead and commit if you have tested on all targets.
Comment 12 Jonas Paulsson 2017-06-23 07:33:39 PDT
(In reply to Simon Pilgrim from comment #11)
> (In reply to Jonas Paulsson from comment #10)
> > Otherwise, one idea is to use -verify-machineinstrs on those tests so that
> > it is possible to always expect the verifier there. If you think this is ok,
> > could I go ahead and commit, or should I open a review?
> 
> This makes sense to me, especially if it means it can stay Generic and be
> tested on all targets. Go ahead and commit if you have tested on all targets.

OK, take a look: r306106