This is an archive of the discontinued LLVM Phabricator instance.

MachineVerifier: Verify liveins list
Needs ReviewPublic

Authored by MatzeB on May 29 2017, 7:43 AM.

Details

Summary
  • Make sure that at the end of basic blocks the liveins of the successor blocks are live (or the CSRs in case of return blocks).
  • Do not allow reserved registers in livein lists, they are unnecessary.
  • Do not allow pristine registers in livein lists, they are unnecessary.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.May 29 2017, 7:43 AM

This is a preview. I am currently cleaning the aarch64 backend for these stricter verifier rules. Esp. the first one is worth putting into LLVM. The 2nd and 3rd can be debated as they aren't technically wrong, though often hint at bad code.

kparzysz edited edge metadata.May 30 2017, 11:20 AM

I don't think we should be aborting on legal code. Maybe we should add an option to treat all verifier reports as warnings (i.e. continue the compilation even if issues were reported)?

Also, didn't you want to eventually get rid of the concept of pristine registers? Doing that would require the opposite---having them as live-ins almost everywhere. Is this patch meant as a short-term solution?

I don't think we should be aborting on legal code. Maybe we should add an option to treat all verifier reports as warnings (i.e. continue the compilation even if issues were reported)?

Also, didn't you want to eventually get rid of the concept of pristine registers? Doing that would require the opposite---having them as live-ins almost everywhere. Is this patch meant as a short-term solution?

Well if we remove the concept we remove the verifier check with it, that's easy :)
So far however seeing pristine registers in livein lists was a good smell for bad code.

I don't think we should be aborting on legal code. Maybe we should add an option to treat all verifier reports as warnings (i.e. continue the compilation even if issues were reported)?

Well looking at the existing situation with targets, we probably have to keep the pristine/reserved checks separate. Just looking at the liveness check currently fails a bunch of lit tests for me:

LLVM :: CodeGen/ARM/2010-05-18-LocalAllocCrash.ll
LLVM :: CodeGen/ARM/2011-04-12-FastRegAlloc.ll
LLVM :: CodeGen/ARM/2011-05-04-MultipleLandingPadSuccs.ll
LLVM :: CodeGen/ARM/2011-08-12-vmovqqqq-pseudo.ll
LLVM :: CodeGen/ARM/ARMLoadStoreDBG.mir
LLVM :: CodeGen/ARM/Windows/dbzchk.ll
LLVM :: CodeGen/ARM/Windows/no-aeabi.ll
LLVM :: CodeGen/ARM/atomic-cmpxchg.ll
LLVM :: CodeGen/ARM/atomic-load-store.ll
LLVM :: CodeGen/ARM/atomic-op.ll
LLVM :: CodeGen/ARM/atomic-ops-v8.ll
LLVM :: CodeGen/ARM/cmpxchg-O0.ll
LLVM :: CodeGen/ARM/coalesce-dbgvalue.ll
LLVM :: CodeGen/ARM/coalesce-subregs.ll
LLVM :: CodeGen/ARM/copy-paired-reg.ll
LLVM :: CodeGen/ARM/crash-greedy.ll
LLVM :: CodeGen/ARM/domain-conv-vmovs.ll
LLVM :: CodeGen/ARM/fast-isel-br-const.ll
LLVM :: CodeGen/ARM/fast-isel-br-phi.ll
LLVM :: CodeGen/ARM/fast-isel-call-multi-reg-return.ll
LLVM :: CodeGen/ARM/fast-isel-call.ll
LLVM :: CodeGen/ARM/fast-isel-cmp-imm.ll
LLVM :: CodeGen/ARM/fast-isel-conversion.ll
LLVM :: CodeGen/ARM/fast-isel-crash.ll
LLVM :: CodeGen/ARM/fast-isel-deadcode.ll
LLVM :: CodeGen/ARM/fast-isel-fold.ll
LLVM :: CodeGen/ARM/fast-isel-frameaddr.ll
LLVM :: CodeGen/ARM/fast-isel-intrinsic.ll
LLVM :: CodeGen/ARM/fast-isel-mvn.ll
LLVM :: CodeGen/ARM/fast-isel-pred.ll
LLVM :: CodeGen/ARM/fast-isel-static.ll
LLVM :: CodeGen/ARM/fast-isel-update-valuemap-for-extract.ll
LLVM :: CodeGen/ARM/fast-isel-vaddd.ll
LLVM :: CodeGen/ARM/fast-isel-vararg.ll
LLVM :: CodeGen/ARM/fast-isel.ll
LLVM :: CodeGen/ARM/func-argpassing-endian.ll
LLVM :: CodeGen/ARM/gpr-paired-spill.ll
LLVM :: CodeGen/ARM/ifconv-kills.ll
LLVM :: CodeGen/ARM/ifcvt-dead-def.ll
LLVM :: CodeGen/ARM/inlineasm-64bit.ll
LLVM :: CodeGen/ARM/ldm-stm-base-materialization.ll
LLVM :: CodeGen/ARM/ldm-stm-i256.ll
LLVM :: CodeGen/ARM/ldrd.ll
LLVM :: CodeGen/ARM/memcpy-ldm-stm.ll
LLVM :: CodeGen/ARM/neon_spill.ll
LLVM :: CodeGen/ARM/optselect-regclass.ll
LLVM :: CodeGen/ARM/pic.ll
LLVM :: CodeGen/ARM/sched-it-debug-nodes.mir
LLVM :: CodeGen/ARM/stack-alignment.ll
LLVM :: CodeGen/ARM/struct_byval_arm_t1_t2.ll
LLVM :: CodeGen/ARM/swift-return.ll
LLVM :: CodeGen/ARM/swifterror.ll
LLVM :: CodeGen/ARM/swiftself.ll
LLVM :: CodeGen/ARM/tail-dup-kill-flags.ll
LLVM :: CodeGen/ARM/thumb-big-stack.ll
LLVM :: CodeGen/ARM/urem-opt-size.ll
LLVM :: CodeGen/Hexagon/ifcvt-live-subreg.mir
LLVM :: CodeGen/Hexagon/livephysregs-lane-masks.mir
LLVM :: CodeGen/Hexagon/livephysregs-lane-masks2.mir
LLVM :: CodeGen/MIR/ARM/cfi-same-value.mir
LLVM :: CodeGen/PowerPC/2006-11-10-DAGCombineMiscompile.ll
LLVM :: CodeGen/PowerPC/2006-12-07-SelectCrash.ll
LLVM :: CodeGen/PowerPC/2008-03-17-RegScavengerCrash.ll
LLVM :: CodeGen/PowerPC/2008-06-23-LiveVariablesCrash.ll
LLVM :: CodeGen/PowerPC/2008-10-28-f128-i32.ll
LLVM :: CodeGen/PowerPC/2011-12-06-SpillAndRestoreCR.ll
LLVM :: CodeGen/PowerPC/2012-09-16-TOC-entry-check.ll
LLVM :: CodeGen/PowerPC/crbit-asm.ll
LLVM :: CodeGen/PowerPC/crbits.ll
LLVM :: CodeGen/PowerPC/ctrloop-sh.ll
LLVM :: CodeGen/PowerPC/ctrloops.ll
LLVM :: CodeGen/PowerPC/expand-contiguous-isel.ll
LLVM :: CodeGen/PowerPC/expand-isel.ll
LLVM :: CodeGen/PowerPC/fast-isel-conversion.ll
LLVM :: CodeGen/PowerPC/float-to-int.ll
LLVM :: CodeGen/PowerPC/fold-zero.ll
LLVM :: CodeGen/PowerPC/fp-to-int-to-fp.ll
LLVM :: CodeGen/PowerPC/i1-ext-fold.ll
LLVM :: CodeGen/PowerPC/i1-to-double.ll
LLVM :: CodeGen/PowerPC/i64_fp_round.ll
LLVM :: CodeGen/PowerPC/ifcvt.ll
LLVM :: CodeGen/PowerPC/int-fp-conv-1.ll
LLVM :: CodeGen/PowerPC/isel.ll
LLVM :: CodeGen/PowerPC/mul-with-overflow.ll
LLVM :: CodeGen/PowerPC/optcmp.ll
LLVM :: CodeGen/PowerPC/p8-isel-sched.ll
LLVM :: CodeGen/PowerPC/ppc-crbits-onoff.ll
LLVM :: CodeGen/PowerPC/ppc32-lshrti3.ll
LLVM :: CodeGen/PowerPC/ppc64-32bit-addic.ll
LLVM :: CodeGen/PowerPC/ppc64-toc.ll
LLVM :: CodeGen/PowerPC/ppcf128sf.ll
LLVM :: CodeGen/PowerPC/pristine-and-livein.mir
LLVM :: CodeGen/PowerPC/select-i1-vs-i1.ll
LLVM :: CodeGen/PowerPC/select_const.ll
LLVM :: CodeGen/PowerPC/subreg-postra.ll
LLVM :: CodeGen/Thumb/2009-08-20-ISelBug.ll
LLVM :: CodeGen/Thumb/2014-06-10-thumb1-ldst-opt-bug.ll
LLVM :: CodeGen/Thumb/and_neg.ll
LLVM :: CodeGen/Thumb/ldm-merge-call.ll
LLVM :: CodeGen/Thumb/ldm-stm-base-materialization-thumb2.ll
LLVM :: CodeGen/Thumb/ldm-stm-base-materialization.ll
LLVM :: CodeGen/Thumb/optionaldef-scheduling.ll
LLVM :: CodeGen/Thumb/tbb-reuse.mir
LLVM :: CodeGen/Thumb/thumb-ldm.ll
LLVM :: CodeGen/Thumb2/constant-islands.ll
LLVM :: CodeGen/Thumb2/crash.ll
LLVM :: CodeGen/Thumb2/large-call.ll
LLVM :: CodeGen/Thumb2/segmented-stacks.ll
LLVM :: CodeGen/X86/block-placement.mir
LLVM :: CodeGen/X86/catchret-fallthrough.ll
LLVM :: CodeGen/X86/crash.ll
LLVM :: CodeGen/X86/implicit-null-check.ll
LLVM :: CodeGen/X86/implicit-null-checks.mir
LLVM :: CodeGen/X86/inline-asm-fpstack.ll
LLVM :: CodeGen/X86/win-catchpad-csrs.ll
LLVM :: CodeGen/X86/win-catchpad-nested-cxx.ll
LLVM :: CodeGen/X86/win-catchpad.ll
LLVM :: CodeGen/X86/win-cleanuppad.ll

Fixing this will be some effort and will take time. I have fixed aarch64 related checks and will look some more into ARM and X86.

But I think this brings us back to a similar discussion as http://llvm.org/PR32146. I think it may be a good idea to add some hooks so targets can indicate their level of verifier cleanliness. This would be a nice bringup tool that would allow us to enable the machine verifier in expensive checks by default or in this case indicate whether we want the "no pristines and no reserved regs in liveins" rule in the verifier enabled yet.

Turns out it isn't even possible to have ARM pass this verifier rule: We currently assume that all CSRs need to be alive at the end of a return block, however LR is modeled as a CSR register, but it does not need to be alive at the end of a return block because it gets copied to PC in the very last instruction.

This was already discovered as part of https://reviews.llvm.org/D33003, I am running into the same problem here.

fhahn added a subscriber: fhahn.Jun 25 2017, 7:41 AM
jonpa added a subscriber: jonpa.Sep 8 2017, 7:19 AM

Make sure that at the end of basic blocks the liveins of the successor blocks are live (or the CSRs in case of return blocks)

So this only checks that there are not any false live-in entries in the live-in lists of successors, meaning that there could still be missing regs in the live-in lists of successors?

I remember a discussion a while back ago to switch to reg-units instead for live-in lists - what is the status on it?

How about using the RDF patch instead of this? I am thinking that since it actually computes the live-ins, it can easily verify the live-in lists. This would also be a way to get RDF into outside Hexagon.

MatzeB added a comment.Sep 8 2017, 9:24 AM

Make sure that at the end of basic blocks the liveins of the successor blocks are live (or the CSRs in case of return blocks)

So this only checks that there are not any false live-in entries in the live-in lists of successors, meaning that there could still be missing regs in the live-in lists of successors?

Missing regs will already be catched (use of an undefined register). The important check here is that regs marked live-in are actually alive at the end of the predecessor blocks.

I remember a discussion a while back ago to switch to reg-units instead for live-in lists - what is the status on it?

No active work on it.

How about using the RDF patch instead of this? I am thinking that since it actually computes the live-ins, it can easily verify the live-in lists. This would also be a way to get RDF into outside Hexagon.

It's a good idea to write separate code for the verifier so bugs don't cancel each other out (it's also deliberately not using LiveIntervals here).

MatzeB updated this revision to Diff 117073.Sep 28 2017, 4:08 PM
  • Rebased on ToT.
  • Now that we have the Restored member on CalleeSavedInfo we can deal with the ARM/LR problems cleanly.
  • Factor out the pristine/reserved register check to a separate commit for now, it's hard enough to get tests passing without them.
  • Various fixes.

This cannot be applied yet as a number of tests fail the new check:
test/CodeGen/ARM/cmpxchg-O0-be.ll
test/CodeGen/ARM/cmpxchg-O0.ll
test/CodeGen/BPF/objdump_intrinsics.ll
test/CodeGen/Hexagon/livephysregs-lane-masks.mir
test/CodeGen/Hexagon/livephysregs-lane-masks2.mir
test/CodeGen/Hexagon/rdf-phi-up.ll
test/CodeGen/PowerPC/2006-11-10-DAGCombineMiscompile.ll
test/CodeGen/PowerPC/2006-12-07-SelectCrash.ll
test/CodeGen/PowerPC/2008-03-17-RegScavengerCrash.ll
test/CodeGen/PowerPC/2008-06-23-LiveVariablesCrash.ll
test/CodeGen/PowerPC/2008-10-28-f128-i32.ll
test/CodeGen/PowerPC/2011-12-06-SpillAndRestoreCR.ll
test/CodeGen/PowerPC/2012-09-16-TOC-entry-check.ll
test/CodeGen/PowerPC/crbit-asm.ll
test/CodeGen/PowerPC/ctrloop-sh.ll
test/CodeGen/PowerPC/ctrloops.ll
test/CodeGen/PowerPC/expand-contiguous-isel.ll
test/CodeGen/PowerPC/fast-isel-conversion.ll
test/CodeGen/PowerPC/float-to-int.ll
test/CodeGen/PowerPC/fold-zero.ll
test/CodeGen/PowerPC/fp-to-int-to-fp.ll
test/CodeGen/PowerPC/i1-ext-fold.ll
test/CodeGen/PowerPC/i1-to-double.ll
test/CodeGen/PowerPC/i64_fp_round.ll
test/CodeGen/PowerPC/ifcvt.ll
test/CodeGen/PowerPC/int-fp-conv-1.ll
test/CodeGen/PowerPC/isel.ll
test/CodeGen/PowerPC/mul-with-overflow.ll
test/CodeGen/PowerPC/p8-isel-sched.ll
test/CodeGen/PowerPC/ppc32-lshrti3.ll
test/CodeGen/PowerPC/ppc64-32bit-addic.ll
test/CodeGen/PowerPC/ppc64-toc.ll
test/CodeGen/PowerPC/ppcf128sf.ll
test/CodeGen/PowerPC/select-i1-vs-i1.ll
test/CodeGen/PowerPC/select_const.ll
test/CodeGen/PowerPC/subreg-postra.ll
test/CodeGen/X86/catchret-fallthrough.ll
test/CodeGen/X86/win-catchpad-csrs.ll
test/CodeGen/X86/win-catchpad-nested-cxx.ll
test/CodeGen/X86/win-catchpad.ll
test/CodeGen/X86/win-cleanuppad.ll