These new debug values get inserted after the place where the spill
happens, which means they won't be reached by the reverse traversal of
basic block instructions. This would crash or fail assertions if they
contained any virtual registers to be replaced. We can manually handle
the new debug values right away to resolve this.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
- I'm somewhat confused that this problem isn't breaking things all over the place and wasn't noticed until now... There must be something unusual here that I don't understand yet...
- This change feels wrong to me: handleDebugValue() is making use of the LiveRegMap infrastructure (findLiveVirtReg) which describes the state at the point of the program where we are currently allocating; However NewDV was created a different position (wherever the previous debug instruction was located that is rewritten).
- I have a feeling that this may be related to DBG_VALUE_LIST / D92578 / CC @StephenTozer as we now suddenly have multiple operands and the problematic vreg operand is an unrelated operand to the spill instruction and is just copied around by buildDbgValueForSpill...
Comment Actions
I'm poking at unfamiliar code here, so I'm definitely not confident that I understand everything going on. I do know that this change is enough to fix the original Rust problems, which now pass without triggering any assertions. It's possible that the resulting debuginfo isn't actually correct though, if I understand your point about the state of LiveRegMap.
Comment Actions
MIR before RegAllocFast:
# *** IR Dump After Two-Address instruction pass (twoaddressinstruction) ***: # Machine code for function _ZN55_$LT$std..io..stdio..Stdin$u20$as$u20$std..io..Read$GT$11read_to_end17haba70a09681d41d3E: NoPHIs, TracksLiveness, TiedOpsRewritten Function Live Ins: $x3 in %2 bb.0 (%ir-block.1): successors: %bb.1, %bb.2 liveins: $x3 %2:g8rc = COPY $x3 %8:g8rc = ANDI8_rec %2:g8rc, 1, implicit-def $cr0 %3:crbitrc = COPY $cr0gt %0:g8rc = LD 0, $zero8 :: (load (s64) from `ptr null`) %1:g8rc = LI8 0 DBG_VALUE_LIST !"iterator", !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value, DW_OP_LLVM_fragment, 64, 64), %0:g8rc, %1:g8rc, debug-location !140; library/alloc/src/vec/spec_extend.rs:0 @[ library/alloc/src/vec/mod.rs:2392:9 @[ library/std/src/io/buffered/bufreader.rs:330:9 @[ library/std/src/io/stdio.rs:464:9 @[ library/std/src/io/stdio.rs:430:9 ] ] ] ] line no:83 %4:crbitrc = CRSET %5:crbitrc = CRXOR %3:crbitrc, killed %4:crbitrc BC killed %5:crbitrc, %bb.2 B %bb.1 bb.1 (%ir-block.4): ; predecessors: %bb.0 BLR8 implicit $lr8, implicit $rm bb.2 (%ir-block.5): ; predecessors: %bb.0 successors: %bb.4, %bb.3 ADJCALLSTACKDOWN 32, 0, implicit-def dead $r1, implicit $r1 %6:g8rc = LI8 0 $x3 = COPY %6:g8rc $x4 = COPY %0:g8rc $x5 = COPY %1:g8rc BL8_NOP &memcpy, <regmask $zero $cr2 $cr3 $cr4 $f14 $f15 $f16 $f17 $f18 $f19 $f20 $f21 $f22 $f23 $f24 $f25 $f26 $f27 $f28 $f29 $f30 $f31 $r14 $r15 $r16 $r17 $r18 $r19 $r20 $r21 $r22 $r23 $r24 and 62 more...>, implicit-def dead $lr8, implicit $rm, implicit $x3, implicit $x4, implicit $x5, implicit $x2, implicit-def $r1, implicit-def $x3 ADJCALLSTACKUP 32, 0, implicit-def dead $r1, implicit $r1 %7:g8rc = COPY $x3 BC %3:crbitrc, %bb.4 B %bb.3 bb.3 (%ir-block.6): ; predecessors: %bb.2 successors: %bb.4 B %bb.4 bb.4 (%ir-block.7): ; predecessors: %bb.2, %bb.3 BLR8 implicit $lr8, implicit $rm # End machine code for function _ZN55_$LT$std..io..stdio..Stdin$u20$as$u20$std..io..Read$GT$11read_to_end17haba70a09681d41d3E.
I've also made the debug print for this failure a bit more precise, and it now says:
Remaining virtual register %0... ...in instruction: DBG_VALUE_LIST !"iterator", !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_deref, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value, DW_OP_LLVM_fragment, 64, 64), %0:g8rc, %stack.0, debug-location !140; library/alloc/src/vec/spec_extend.rs:0 @[ library/alloc/src/vec/mod.rs:2392:9 @[ library/std/src/io/buffered/bufreader.rs:330:9 @[ library/std/src/io/stdio.rs:464:9 @[ library/std/src/io/stdio.rs:430:9 ] ] ] ] line no:83 ...in instruction: DBG_VALUE_LIST !"iterator", !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_deref, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value, DW_OP_LLVM_fragment, 64, 64), %0:g8rc, %stack.0, debug-location !140; library/alloc/src/vec/spec_extend.rs:0 @[ library/alloc/src/vec/mod.rs:2392:9 @[ library/std/src/io/buffered/bufreader.rs:330:9 @[ library/std/src/io/stdio.rs:464:9 @[ library/std/src/io/stdio.rs:430:9 ] ] ] ] line no:83
So yes @MatzeB, this is related to DBG_VALUE_LIST.
Comment Actions
Apologies for taking a bit of time to get to this. My thoughts below, tl;dr it's probably better to just skip tracking spills for DBG_VALUE_LISTs in RegAllocFast.
The fix is reasonable in that it causes all of the debug operands to be processed for the DBG_VALUE_LIST, but from what I can see I think the issue is just that RegAllocFast does not handle debug info in optimized code well. Some of the behaviour results in incorrect behaviour in the presence of optimized code - it tracks the set of live debug values for a given register, but does not account for earlier debug values being killed by later debug values, so produces potentially large bundles of redundant debug instructions (which unfortunately this fix would add to, as any newly produced debug values would be added to the list of debug values to repeatedly clone). Cloning debug values to the end of the basic block may also result in earlier debug values overwriting later debug values, producing incorrect debug info. This isn't normally a problem in unoptimized code, where we generally have a single DBG_VALUE for each variable and no DBG_VALUE_LISTs, which is what I believe RegAllocFast is normally expecting. It looks as though this is a somewhat unusual case in that the IR is optimized, but llc is being invoked with -O0; it's worth noting that FastIsel simply drops DBG_VALUE_LISTs outright without trying to produce an instruction.
If RegAllocFast is to handle these cases correctly then some effort could be put into fixing it by adding more support for tracking operands of debug values, but I don't think it would be worth it. Besides being somewhat outside of what RegAllocFast is intended to handle AFAIK, the problem of tracking logical values through machine locations throughout the program is non-trivial and should be solved in the LiveDebugValues pass as much as possible. For DBG_VALUE and DBG_VALUE_LIST instructions it is currently necessary for the register allocator to do some of this work in order to prevent certain debug values from being killed when we rewrite them to use a physical registers or spill slot, but this is more generally solved by the debug info instruction referencing work, which removes the need to track spills/restores prior to LiveDebugValues; support for multi-location debug values in DBG_INSTR_REF is not currently present but is being worked on and due to land soon, which should resolve this issue if instruction referencing is enabled for the build in question.
| llvm/lib/CodeGen/RegAllocFast.cpp | ||
|---|---|---|
| 452–471 | See full review comment. DBG_VALUE_LIST still needs to be handled in handleDebugValue to ensure its operands are updated correctly, but this change means that it won't be updated through spills. | |
Comment Actions
I had changed my commit message to reflect that we're now skipping these, instead of handling them, but arc amend changed it back when I only meant to update the review status. I suppose I should have updated the title+summary here too -- sorry about that! For posterity, here's what I wanted it to say, dug out of my reflog:
[RegAllocFast] Skip debug value lists for spills New debug values for spills get inserted after the place where the spill happens, which means they won't be reached by the reverse traversal of basic block instructions. This would crash or fail assertions if they contained any virtual registers to be replaced. We don't have enough support for tracking operands of debug values to properly update these, so just skip DBG_VALUE_LIST for spills. This only should only arise for the unusual case of optimized IR passed to O0 codegen.
See full review comment. DBG_VALUE_LIST still needs to be handled in handleDebugValue to ensure its operands are updated correctly, but this change means that it won't be updated through spills.