[PATCH] D112852: [GlobalISel] Allow DBG_VALUE to use undefined vregs before LiveDebugValues

[Jack Andersen via Phabricator via llvm-commits]

jackoalan added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:1992
+        if (isFunctionTracksDebugUserValues || !MO->isUse() ||
+            !MI->isDebugValue() || !MRI->def_empty(Reg)) {
+          // If we're post-Select, we can't have gvregs anymore.
----------------
bogner wrote:
> jackoalan wrote:
> > bogner wrote:
> > > jackoalan wrote:
> > > > bogner wrote:
> > > > > Is this check in the right place? I'm seeing "Generic virtual register use cannot be undef" (just above) errors after this change on an out of tree target.
> > > > Does the error occur after the Debug Variable Analysis pass on a debug instruction?
> > > > 
> > > > If possible, it would be really helpful if a test could be created that triggers the error on the main branch.
> > > The error occurs before the Debug Variable Analysis pass runs when running llc with `-verify-machineinstrs`, since that runs the machine verifier between each pass. I can try to see if I can find a reproducer for an in-tree backend, but I figured I'd sanity check this first, given that the comments says "Debug value instruction is permitted to use undefined vregs.", but the check that rejects undef vregs happens right before this.
> > Okay, I see what you're referring to.
> > 
> > The purpose of this added if block is to weaken the verifier just for DBG_VALUE register uses; making it convenient to erase instructions that define these values but then not explicitly undef-ing them on the DBG_VALUE use (i.e. just a dangling vreg). In this line of thinking, it would not be necessary to also weaken `MO->isUndef` (which only tests for explicit undefs).
> > 
> > Does the error contain a dump of the instruction? If it is indeed a DBG_VALUE or DBG_VALUE_LIST instruction, then `MO->isUndef()` should be weakened as well if it is determined there is no other root cause. If it is not a DBG_VALUE instruction, then it would not be applicable and the root cause must be elsewhere.
> > 
> > A triggering test would be the best way to determine this collaboratively.
> The errors look something like this:
> ```
> *** Bad machine code: Generic virtual register use cannot be undef ***
> - function:    xyz
> - basic block: %bb.4 while.end64.i.i (0x7fdf95dc8c48)
> - instruction: DBG_VALUE undef %2423:gpr, $noreg, !"x", !DIExpression(DW_OP_constu, 0, DW_OP_swap, DW_OP_xderef), debug-location !3302; /...:184:30 @[ program_source:273:69 @[ program_source:0 ] ] line no:184
> - operand 0:   undef %2423:grp
> ```
> Let me see if I can reduce this test to something shareable.
Have you tried using bugpoint to reduce your crashing function to something you can share? You may have to anonymize identifiers, but it usually does a good job minimizing the IR to just the blocks and instructions necessary to crash.

I’d be happy to weaken that undef test for DBG_VALUE, but It’d be ideal to justify it with a test if that change is made.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112852/new/

https://reviews.llvm.org/D112852