Page MenuHomePhabricator

[WIP][DebugInfo] Avoid SelectionDAG un-necessarily debug-referring to dead VRegs
AbandonedPublic

Authored by jmorse on Apr 26 2019, 3:57 AM.

Details

Reviewers
bjope
Summary

[Not for review or submission yet, aim is to evaluate this first]

Hi Bjorn,

This patch hopefully alleviates some of the regressions seen with D56151 / PR40010, where unfortunate DBG_VALUE creation by SelectionDAG generates debug uses of dead VRegs, that then get dropped later. More information on what's going on is in PR41583: this patch causes SelectionDAG to point/place DBG_VALUEs at the VReg/instruction where an exported Value is written to its long-term VReg, rather than where the Value is computed.

This isn't a panacea: fractionally more variable locations get dropped in a clang-3.4 build, largely because of long-term VRegs subsequently being optimised out by OptimizePHIs and similar passes. In effect this change would swap (assuming D56151 is applied) locations dropped due to liveness issues, for locations dropped due to long-term VRegs being optimised out. (For me, the latter is more preferable).

There's the possibility that this won't resolve the regressions seen with D56151 if the problem really hinges on register class copies: I haven't managed to replicate such a scenario on X86 or AArch64 as they have simple register hierarchies. The problem would be analogous (differences between value-computed and value-used VRegs and locations) but would be more unpleasent to solve.

Diff Detail

Event Timeline

jmorse created this revision.Apr 26 2019, 3:57 AM
aprantl added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1272

Comment that explains what this function does?

1283

Perhaps:

if (!RFV.occupiesMultipleRegs()) {
   SDV = DAG.getVRegDbgValue(Var, Expr, Reg, Node, dl, Order);
   DAG.AddDbgValue(SDV, Node, false);
   return;
}
...
1286

unsigned BitsToDescribe = Var->getSizeInBits().getValueOr(0);

jmorse updated this revision to Diff 198622.May 8 2019, 4:18 AM

Scatter comments / address review

jmorse marked 3 inline comments as done.May 8 2019, 4:19 AM
jmorse updated this revision to Diff 198632.May 8 2019, 5:48 AM

Remove an accidental isIndirect=true flag, my bad.

NB, this patch does make four tests fail, all due to subtle shifts in DBG_VALUE placement (which is expected). I'll add the updates for those if this turns out to be a profitable direction.

aprantl added inline comments.May 8 2019, 9:28 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1276

///

1291

early return?

bjope added a comment.May 14 2019, 8:57 AM

Sorry. I realize that I've completely forgotten to look at this. I'll try to find some time before the end of the week.

jmorse updated this revision to Diff 204998.Jun 17 2019, 12:58 AM

Address feedback / review

Sort-of ping at @bjope on this: this is fundamentally a speculative patch as I can't dig into the regressions being seen in D56151, and I have no expectations on other peoples time; however we'll need to think about whether those regressions block the placeDbgValues chain-of-things going into llvm-9, which will branch sometime soon (next month?). Similar to what I wrote in D56151, we have a choice of poisons for llvm-9:

  • Leave placeDbgValues as it is,
  • Disable placeDbgValues but don't commit D56151, which may cause dead variable locations to be resurrected during register coalescing,
  • Disable placeDbgValues, commit D56151, triggering the regressions @bjope saw there, which may or may not be alleviated by this patch.

I'm fairly confident the issue in PR41583 (which I believe causes the seen regressions) can be addressed comprehensively, but likely not quickly. Delaying all these things doesn't bother me (fixing placeDbgValues has uncovered all kinds of other fun), but it should be a proactive decision.

jmorse abandoned this revision.Jun 27 2019, 3:24 AM

Abandoning this WIP -- addressing the "values are in two places in SelectionDAG" problem isn't going to be so easy to solve.