Page MenuHomePhabricator

[DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations
ClosedPublic

Authored by jmorse on Dec 29 2018, 10:12 AM.

Details

Summary

This is a fix for PR40010 [0]. DBG_VALUE instructions do not contribute to the liveness of virtual registers, making them invisible to the register coaleser, which may merge new register valuations over the top of DBG_VALUEs. When this happens, new valuations can appear to debuggers that were never in the source program, which is misleading.

To avoid this, examine the debug users of the register being eliminated, and pick out any that:

  1. Lie in a location where the register isn't live, and
  2. Where the destination register _is_ live at that location, and has a different def.

Which means the DBG_VALUE definitely gets resurrected, with a different value. This patch can leave DBG_VALUEs of non-live registers so long as the register isn't written to.

Note this also marks use-before-any-def's as undef, if they're coalesced to a different register, as it's unclear to me how to establish which def a DBG_VALUE is supposed to refer to in that case.

[0] https://bugs.llvm.org/show_bug.cgi?id=40010

Diff Detail

Event Timeline

jmorse created this revision.Dec 29 2018, 10:12 AM
jmorse planned changes to this revision.Dec 29 2018, 12:08 PM

Hmmm. In retrospect, aiming to cover all cases where there's a def of the dest-reg in between the last src-def and the DBG_VALUE means considering control flow, which I haven't done.

It's probably better for now to only account for circumstances where reg-coalescing makes the DBG_VALUE live again, and thus we can be 100% confident it does the wrong thing. That more closely matches the test case uploaded.

jmorse updated this revision to Diff 179705.Dec 29 2018, 12:52 PM
jmorse edited the summary of this revision. (Show Details)

Target only DBG_VALUEs that are definitely resurrected with a different def by register coalescing.

vsk added a comment.Dec 31 2018, 2:32 PM

Thanks for working on this.

As you pointed out in llvm.org/PR40010, we do have a general problem of leaving around DBG_VALUE instructions which should be dead/undef but aren't. Do you think it would be possible/worthwhile to diagnose this issue in MachineVerifier?

lib/CodeGen/RegisterCoalescer.cpp
1900

Using MachineRegisterInfo::reg_instructions might aid readability here. Any reason not to?

1906

Could you explain why it's sufficient to fix up coalescer pairs in which the destination is a vreg?

jmorse added a comment.Jan 3 2019, 3:28 AM

Thanks for the review,

In D56151#1343002, @vsk wrote:

As you pointed out in llvm.org/PR40010, we do have a general problem of leaving around DBG_VALUE instructions which should be dead/undef but aren't. Do you think it would be possible/worthwhile to diagnose this issue in MachineVerifier?

IMHO that's a definite yes, eventually. It's non-trivial for a human to see where liveness starts and ends for DBG_VALUEs, but a significant liability for optimisations as they can't "see" the DBG_VALUEs they're messing with. A truly dead DBG_VALUE will be dropped anyway, because it has no meaning, so it's not a useful feature to keep.

I'm not familiar with the machine verifier, but it appears to check liveness for things like kill flags, and just ignores debug users, so enabling checking of DBG_VALUEs should be easy to implement.

(I'm preparing a patch that recovers dead DBG_VALUEs from still-live copies, which would have to change if we made dead DBG_VALUEs illegal, but that's a different matter).

lib/CodeGen/RegisterCoalescer.cpp
1900

No reason (lack of familiarity), I'll switch to that,

1906

Hmmm. I've been operating on the assumption that the coalescer only operates on vregs, but I now see that isn't necessarily the case... I'll investigate this one.

jmorse planned changes to this revision.Jan 3 2019, 8:08 AM

On the topic of coalescing physical registers, it appears the liveness test is well defined in joinReservedPhysReg. However while absorbing all of this, I've realised that dead DBG_VALUEs of both src and dst registers can be resurrected when their intervals are joined. I'll attempt to generalise further...

jmorse updated this revision to Diff 180216.Jan 4 2019, 3:30 AM

Here's a further revision. Two significant changes:

  • Because DBG_VALUEs of either the source or destination register can be resurrected by coalescing, check both,
  • There's now no attempt to check whether the two registers refer to the same value number.

Examining physical registers is only performed for the destination regnum as the CoalescerPair class cannonicalises to make any physreg the destination.

LiveIntervals isn't as strong an analysis as I'd thought, and it looks like my attempt in the previous revision to find the value the DBG_VALUE referred to before it was killed, will break in the presence of any control flow. The patch as it is will now make DBG_VALUEs undef even if the coalescing by coincidence resurrects it to the same value. IMHO this is a reasonable trade-off when we can't prove the resurrection is correct.

On a stage2 build of llvm/clang r349779 a trivial fraction of variable locations are lost by this (less than 0.01%).

It'd be great to revert this patch if we reach a state where dead DBG_VALUEs can't make it as far as simple-register-coalescing in the future.

aprantl added inline comments.Jan 4 2019, 7:29 AM
lib/CodeGen/RegisterCoalescer.cpp
248

valuation is not a term we typically use in this context? Perhaps should the operands of the DBG_VALUE be updated?

jmorse updated this revision to Diff 181530.Jan 14 2019, 4:29 AM

Revise comment for mergeChangesDbgValue

jmorse marked an inline comment as done.Jan 14 2019, 4:32 AM
jmorse added inline comments.
lib/CodeGen/RegisterCoalescer.cpp
248

Updated with some new text -- I've gone for "would the def that a DBG_VALUE refers to change?", which should be precise about what the function tests for.

bjope added inline comments.Jan 14 2019, 12:10 PM
test/CodeGen/X86/pr40010.mir
82

I think you can remove all the false initializations here (keep tracksRegLiveness: true).

See: https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files

89

I think the registers: section can be removed (these mapping are given by the MIR below, right?
And liveins, fixedStack, stack, constants can also be removed here, right?

See: https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files

115

Maybe easier to read if you put all CHECK:s for the basic block here (consecutive lines)?
Or all checks for the function just before/after the function (might need to use # CHECK: instead of ; CHECK: depending on context.

(I've actually never used ; CHECK: within the body like this so I did not know that it works like that.)

I would probably have used something like this:

# <description of what the test is supposed to verify>
# 
# CHECK-LABEL: name:            test1
# CHECK: bb.1:
# CHECK:   %7:gr32 = ADD32rr %7
# CHECK:   DBG_VALUE $noreg
# CHECK: bb.2:

Where the CHECK-LABEL is supposed to make sure that each subtest is self-contained (do not match with anything from another subtest). This also makes it possible to skip some of the IR (since you do not need the symbolic names for the basic blocks.

jmorse updated this revision to Diff 181795.Jan 15 2019, 8:40 AM

Trim and revise MIR test as per Björn's comments.

I've put the block of CHECKs together, which looks a lot better now.

aprantl added inline comments.Jan 15 2019, 8:52 AM
lib/CodeGen/RegisterCoalescer.cpp
1628

clang-format?

jmorse updated this revision to Diff 181804.Jan 15 2019, 9:17 AM
jmorse marked an inline comment as done.

clang-format-diff, whoops.

TWeaver marked an inline comment as done.Jan 21 2019, 9:35 AM
TWeaver added a subscriber: TWeaver.
TWeaver added inline comments.
lib/CodeGen/RegisterCoalescer.cpp
1724

I believe theres a typo in this comment

We now "have" whether...

should be

We now "know" whether...

jmorse updated this revision to Diff 183526.Jan 25 2019, 5:55 AM

Fix a comment wording.

qcolombet added inline comments.Feb 25 2019, 8:40 AM
test/CodeGen/X86/pr40010.mir
2

Use a filename that describes the problem briefly rather than referencing the PR number.

jmorse updated this revision to Diff 188914.Mar 1 2019, 7:17 AM

Rename new test case's file name, move it to the 'DebugInfo' directory too as that seems more appropriate.

Ping on this -- I think we all agree that this is a part of a large issue (DBG_VALUEs of non-live variables), but this patch is a step in the right direction.

bjope added a comment.Mar 26 2019, 9:05 AM

FYI: I've downloaded this patch and have been running some tests (on a C code base). Lots of DBG_VALUE:s that now becomes undef. Some of them were totally wrong before (so this patch is good). But in some situations it looks like a regression. Need some more time to analyse those to find out if there is something wrong with mergingChangesDbgValue (that can be easily fixed), or to understand better why we get those regressions.

The defensive approach would of course be to land this, since it obviously fixes some "hard" faults while getting "optimized out" isn't a fault (even if it might be considered a regression). So I just want to understand that it isn't some obvious fault with the liveness checks that makes this just randomly turning DBG_VALUE:s into undef before I approve this.

bjope added a comment.Mar 27 2019, 3:02 AM

I'll try to describe one "regression" that I've seen.

Before ISel we've got

...
%0 = tail call %struct. @llvm.phx.divm.u16.s_struct.s(i16 %a, i16 %b), !dbg !708
%.fca.0.extract = extractvalue %struct. %0, 0, !dbg !708
call void @llvm.dbg.value(metadata i16 %.fca.0.extract, metadata !686, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 16)), !dbg !709
%.fca.1.extract = extractvalue %struct. %0, 1, !dbg !708
call void @llvm.dbg.value(metadata i16 %.fca.1.extract, metadata !686, metadata !DIExpression(DW_OP_LLVM_fragment, 16, 16)), !dbg !709
...
use %.fca.0.extract
use %.fca.1.extract
...

After ISel it looks like this

...
%14:anh_0_7, %15:anh_0_7 = divm16_pseudo killed %13:anh_0_7, %7:anh_0_7, implicit-def dead $ccreg, debug-location !708; 
DBG_VALUE %14:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
%0:anh_rn = COPY %15:anh_0_7, debug-location !708;
DBG_VALUE %15:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
...
use %14:anh_0_7
use %0:anh:rn
...

Maybe ISel should have put the COPY to %0 after the last DBG_VALUE (now we got a dbg-use of %15 after the last non-dbg-use of %15). Or maybe it should have used %0 instead of %15 in that DBG_VALUE. Or maybe there should be one DBG_VALUE before the COPY using %15 and one after using %0.

Before simple register coalescing we get this (some more COPY instructions after expanding the pseudo that is using some hard coded physical registers):

...
208B	  divm16 0, $noreg, 0, implicit-def $a4_40, implicit-def dead $af4, implicit-def dead $ccreg, implicit-def $a5_40, implicit $a4h, implicit $a5h, debug-location !708;
224B	  %42:an40_0_7 = COPY $a4_40, debug-location !708;
240B	  %14:anh_0_7 = COPY %42.hi16:an40_0_7, debug-location !708;
256B	  %43:an40_0_7 = COPY $a5_40, debug-location !708;
272B	  %15:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
	  DBG_VALUE %14:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
288B	  %0:anh_0_7 = COPY %15:anh_0_7, debug-location !708;
	  DBG_VALUE %15:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
...
          use %14:anh_0_7
          use %0:anh_0_7
...

Without this patch we get

...
208B	  divm16 0, $noreg, 0, implicit-def $a4_40, implicit-def dead $af4, implicit-def dead $ccreg, implicit-def $a5_40, implicit $a4h, implicit $a5h, debug-location !708;
224B	  %42:an40_0_7 = COPY $a4_40, debug-location !708;
256B	  %43:an40_0_7 = COPY $a5_40, debug-location !708;
	  DBG_VALUE %42.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
	  DBG_VALUE %43.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
...
          use %42.hi16:an40_0_7
          use %43.hi16:an40_0_7
...

With the patch the last DBG_VALUE is changed into using %noreg, so instead we get

...
208B	  divm16 0, $noreg, 0, implicit-def $a4_40, implicit-def dead $af4, implicit-def dead $ccreg, implicit-def $a5_40, implicit $a4h, implicit $a5h, debug-location !708;
224B	  %42:an40_0_7 = COPY $a4_40, debug-location !708;
256B	  %43:an40_0_7 = COPY $a5_40, debug-location !708;
	  DBG_VALUE %42.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
	  DBG_VALUE %noreg.hi16, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
...
          use %42.hi16:an40_0_7
          use %43.hi16:an40_0_7
...

It is when analyzing the COPY to %0 that we detect that the DBG_VALUE is unsound:

272B	%15:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
	Considering merging to aN40_0_7 with %15 in %43:hi16
		RHS = %15 [272r,288r:0)  0@272r weight:0.000000e+00
		LHS = %43 [256r,272r:0)  0@256r weight:0.000000e+00
		merge %15:0@272r into %43:0@256r --> @256r
		erased:	272r	%15:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
		updated: 288B	%0:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
		updated: DBG_VALUE %43.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
Shrink: %43 [256r,288r:0)  0@256r weight:0.000000e+00
Shrunk: %43 [256r,288r:0)  0@256r weight:0.000000e+00
	Success: %15:hi16 -> %43
	Result = %43 [256r,288r:0)  0@256r weight:0.000000e+00

288B	%0:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
	Considering merging to aN40_0_7 with %0 in %43:hi16
		RHS = %0 [288r,928r:0)  0@288r weight:0.000000e+00
		LHS = %43 [256r,288r:0)  0@256r weight:0.000000e+00
		merge %0:0@288r into %43:0@256r --> @256r
		erased:	288r	%0:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
		updated: 928B	%54:anh_0_7 = subs_a16_a16_a16 %40:anh_0_7, %43.hi16:an40_0_7, 0, $noreg, 0, implicit-def dead $ccreg, implicit $cuc, debug-location !758;
Update of %43.hi16:an40_0_7 would be unsound, setting undef

So we have already merged aN40_0_7 with %15 in %43:hi16 and updated the DBG_VALUE to use %43:hi16 before we find it unsound. So in the past this coalescing made the DBG_VALUE sound (as it extended the live range for %43 to cover the DBG_VALUE). But with this patch we instead detect the DBG_VALUE as being unsound.

Inside mergingChangesDbgValue I can see

DbgV->dump() =>   DBG_VALUE %43.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
DbgReg = 2147483691 (%43)
DstReg = 2147483691 (%43)
SrcReg = 2147483648 (%0)
SrcLive = true
DstLive = false

That is basically how far I've analysed this scenario at the moment.

Some thoughts:

  • Should this be seen as a fault in ISel? We could probably get a better result if ISel worked differently. But in general I think we allow dbg-uses outside the non-dbg-live-interval, so I do not think it really is a fault.
  • A late dbg-use (for an otherwise killed reg) is only unsound if the register allocator is using the allocated physical register for some other purpose. Isn't it a little bit early to detect that in the register coalescer? Maybe we need to take the register allocation into account if we want to make a better solution. Isn't LiveDebugVariables supposed to handle this?
  • Is this patch really supposed to detect this DBG_VALUE as unsound? (we are not updating %43 here, we are only extending the live range to cover the DBG_VALUE that already is using %43). Should perhaps mergingChangesDbgValue return false also when (SrcLive && DstReg == DbgReg), or could this be unsound?
bjope added a comment.Mar 27 2019, 4:54 AM
  • Is this patch really supposed to detect this DBG_VALUE as unsound? (we are not updating %43 here, we are only extending the live range to cover the DBG_VALUE that already is using %43). Should perhaps mergingChangesDbgValue return false also when (SrcLive && DstReg == DbgReg), or could this be unsound?

I can probably answer that myself. This is after de-SSA (for awhile I didn't consider that), so I guess this is more or less exactly what this patch is supposed to detec. We do not know that SrcReg has the same value as DstReg for the full live range of SrcReg and DstReg (only at the COPY). We just merge the live ranges, but the register could have different values throughout the program. Unfortunately for my example program this will be seen as a regression since there are no redefinition of SrcReg/DstReg between the COPY and the DBG_VALUE. And the FIXME you have added on line 1974 probably covers that case.

Maybe ISel should have put the COPY to %0 after the last DBG_VALUE (now we got a dbg-use of %15 after the last non-dbg-use of %15). Or maybe it should have used %0 instead of %15 in that DBG_VALUE. Or maybe there should be one DBG_VALUE before the COPY using %15 and one after using %0.

Ugh, I remember debugging into this issue now (it then fell out of my short term memory). IIRC:

  • All Values used between blocks get a vreg allocated,
  • When the DAG is converted to MIR instructions, each instruction (with a def) gets its own vreg number
  • CopyToReg nodes then copy from a local vreg to a cross-block vreg.

In various cases this means a DBG_VALUE gets attached to a block-local vreg that immediately gets copy-killed to the cross-block vreg, which this patch then has difficulties with. Given that this is, more or less, a design feature of SelectionDAG, I guess it should be considered An Issue (TM).

Should this be seen as a fault in ISel? We could probably get a better result if ISel worked differently. But in general I think we allow dbg-uses outside the non-dbg-live-interval, so I do not think it really is a fault.

It seems to be part of the design, and I don't know SelectionDAG well enough to determine whether changing it would break assumptions elsewhere. Assuming my summary above is correct, a quick fix might be to prefer allocated vreg locations over even SDNodes when processing dbg.value intrinsics, but I haven't considered what else might go wrong.

The real trouble is that the DBG_VALUEs are correct while the MIR function is in SSA form, but when we leave it and consider liveness, they can become incorrect (or non-live at least).

A late dbg-use (for an otherwise killed reg) is only unsound if the register allocator is using the allocated physical register for some other purpose. Isn't it a little bit early to detect that in the register coalescer? Maybe we need to take the register allocation into account if we want to make a better solution. Isn't LiveDebugVariables supposed to handle this?

Alas, I believe the register coalescer is doing some of the register allocators job: consider this example, produced by mangling test2 in the added regression test:

bb.0.entry:
  successors: %bb.1(0x80000000)
  liveins: $rdi

  %1:gr32 = COPY $rdi
  %3:gr32 = MOV32r0 implicit-def dead $eflags

bb.1.start.test2:
  successors: %bb.1(0x7c000000)

  %0:gr32 = PHI %50, %bb.1.start.test2, %3, %bb.0.entry
  %50:gr32 = XOR32rr %0, %1, implicit-def dead $eflags
  DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5
  JMP_1 %bb.1

Full file here: https://pastebin.com/HqkCB6Bi . Important feature is that the DBG_VALUE refers to %0, which is not live. Note that XOR32rr is a two-address instruction. If I run "llc -simplify-mir in.mir -start-before=phi-node-elimination -stop-before=simple-register-coalescing -o -", the value of %0 is preserved by the two-address-instruction pass through a copy, and the DBG_VALUE refers to that. But then if you change "stop-before" to "stop-after", you can see the coalescer merges %0 and %50, and the DBG_VALUE now refers to a live copy of the result of the XOR, not the input to it. (With this patch applied, I don't believe it does). Once the coalescer has run, there's no way to recover the original behaviour AFAIK.

LiveDebugVariables discards all non-live DBG_VALUEs [0]. I tried to fight against this for a bit, but then found bugs like this (this patch) cropping up.

~

Given that we've identified this now, it's probably best to keep this & the associated patches hanging until there's a fix for this behaviour (the local/cross-function vreg copying that is). Which is awkward, but it's ages until LLVM9 branches.

[0] https://github.com/llvm-mirror/llvm/blob/19a56211e133d6981fca86913ca6b97a701cee52/lib/CodeGen/LiveDebugVariables.cpp#L626

NB: I've written up a sort of broad-ish summary of what's wrong with the SelectionDAG scheduling of DBG_VALUEs in PR41583 [0], which IMO is the cause of the unfortunate variable-location droppage described above. I've got an additional patch that should go up tomorrow.

[0] https://bugs.llvm.org/show_bug.cgi?id=41583

Hi, I've uploaded D61181 with a patch that might fix those regressions -- if you happen to have any spare cycles for evaluating it that'd be appreciated.

If that patch doesn't work well though, then we're likely stuck in the tar pit with some unfortunate facts:

  • Avoiding debug uses of non-live VRegs would be difficult to fix quickly,
  • This patch here would lead to debug regressions if committed,
  • Without this patch, disabling placeDbgValues (D58453) and the resulting greater distribution of dbg.values throughout functions would be liable to the failure mode this patch tries to fix.

Ping: we should hammer out this review, seeing how the branch date for llvm-9 has been announced. Could I suggest that everyone is happy with the *implementation* of this patch, but that it's not yet agreed that it needs merging?

The patch effectively makes the codegen backend stricter about what DBG_VALUE insts it will accept, i.e. they have to refer to vregs that are alive. If it helps, this is already something that's enforced a few optimisation passes later in LiveDebugVariables [0], this patch just brings that check further forwards, due to the errors that can be introduced (described above). What's unfortunate is that this also prevents occasions where register coalescing re-enlivens DBG_VALUEs with a vreg that has the right value in it, occasions which can be generated due to limitations in SelectionDAG.

This then becomes a trade-off (see immediately preceeding comment) of whether the problem is so bad that we should trade some lost locations for getting rid of placeDbgValues. IMHO: yes, because we're trading definitely-incorrect locations for some more being optimized-out, a net reduction in error, to me. Other opinions most welcome.

[0] https://github.com/llvm/llvm-project/blob/b9f1e7b16ed2341e54b4e2033d111e7a2ca19b9a/llvm/lib/CodeGen/LiveDebugVariables.cpp#L624

Ping: we should hammer out this review, seeing how the branch date for llvm-9 has been announced. Could I suggest that everyone is happy with the *implementation* of this patch, but that it's not yet agreed that it needs merging?

The patch effectively makes the codegen backend stricter about what DBG_VALUE insts it will accept, i.e. they have to refer to vregs that are alive. If it helps, this is already something that's enforced a few optimisation passes later in LiveDebugVariables [0], this patch just brings that check further forwards, due to the errors that can be introduced (described above). What's unfortunate is that this also prevents occasions where register coalescing re-enlivens DBG_VALUEs with a vreg that has the right value in it, occasions which can be generated due to limitations in SelectionDAG.

This then becomes a trade-off (see immediately preceeding comment) of whether the problem is so bad that we should trade some lost locations for getting rid of placeDbgValues. IMHO: yes, because we're trading definitely-incorrect locations for some more being optimized-out, a net reduction in error, to me. Other opinions most welcome.

[0] https://github.com/llvm/llvm-project/blob/b9f1e7b16ed2341e54b4e2033d111e7a2ca19b9a/llvm/lib/CodeGen/LiveDebugVariables.cpp#L624

I'm reading this as: "this patch make debug info more accurate by rejecting invalid DBG_VALUEs earlier on, but it overshoots the target a bit and also rejects a few(?) false positives". I support this approach, as we all want to move LLVM into the "more accurate" direction, assuming that the false positives are fixable and the ratio of rejected incorrect DBG_VALUEs to false positives is favorable.

Hi,

I'm reading this as: "this patch make debug info more accurate by rejecting invalid DBG_VALUEs earlier on, but it overshoots the target a bit and also rejects a few(?) false positives". I support this approach, as we all want to move LLVM into the "more accurate" direction, assuming that the false positives are fixable and the ratio of rejected incorrect DBG_VALUEs to false positives is favorable.

Exactly that; on x86_64 at least from my comment here [0] the loss in variable locations is small (possibly it's larger for Bjorns arch?), but the false-positives are very obvious to the human eye, which is annoying. I don't have a feeling for the true/false ratio (I can dig into it), but again the impact is small.

(As I recall, there was no way to detect the false-positives without adding a new MachineDominatorTree analysis, which seemed excessive when we shouldn't be generating the faulty DBG_VALUEs in the first place).

[0] https://reviews.llvm.org/D56151#1346195

aprantl accepted this revision.Jun 26 2019, 11:28 AM
aprantl added inline comments.
lib/CodeGen/RegisterCoalescer.cpp
1695

source register

1709

Constant physical registers

This revision is now accepted and ready to land.Jun 26 2019, 11:28 AM
bjope added a comment.Jun 27 2019, 2:33 AM

LGTM as well.

I'll also share some stats after running some tests last night:

Compiled 4458 C files (using our DSP-C frontend and customized DSP backend).
Without this patch we get 1791988 "DEBUG_VALUE" comments in the resulting .s files, with the patch 1791453 (-535).
By doing a simple diff between all the .s files it seems like 753 old DEBUG_VALUE comments have been replaced by 218 new ones. And out of the new ones 183 now got "undef" as the value.

I haven't looked through all of those in detail (and I do not intend to do it either right now, not always easy to map things back to the source code to understand if it is "more correct" now or not).
What I've seen is that there are a few cases where I believe the old DEBUG_VALUE was correct and now we get "undef", and some cases where it looks like we would get faulty debug info without this patch.

FWIW, this analysis was of course not perfect when it comes to comparing the quality of debug info, but it gives some kind of measure.
My feeling is that the cases where we now lose some debug info is quite rare (at least for our DSP-C/DSP target), and the benefit of making the register coalescers handling of DBG_VALUE more sound outweigh that problem.

Bjorn wrote:

My feeling is that the cases where we now lose some debug info is quite rare (at least for our DSP-C/DSP target), and the benefit of making the register coalescers handling of DBG_VALUE more sound outweigh that problem.

Awesome -- now dropping this in with a minor test change (JB_1 -> JCC_1) and comment fixes. Many thanks for all the reviews!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 3:23 AM