Page MenuHomePhabricator

[DebugInfo] Insert DEBUG_VALUEs after each register redefinition
ClosedPublic

Authored by Ka-Ka on Sep 25 2017, 2:52 AM.

Details

Summary

When reinserting debug values after register allocation, make sure to
insert debug values after each redefinition of debug value register in
the slot index range. The reason for this is that DwarfDebug will end
the range of a debug variable when the physical reg is defined. For
instructions with e.g. tied operands this result in prematurely ended
debug range.

This resolves pr34545

Patch by Karl-Johan Karlsson and Bjorn Pettersson

Diff Detail

Repository
rL LLVM

Event Timeline

Ka-Ka created this revision.Sep 25 2017, 2:52 AM
bjope added a subscriber: bjope.Sep 25 2017, 4:21 AM
rnk edited edge metadata.Sep 25 2017, 8:30 AM

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

aprantl edited edge metadata.Sep 25 2017, 9:13 AM
In D38229#880149, @rnk wrote:

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

In LiveDebugValues we use a heuristic based on the lexical scopes to avoid inserting DBG_VALUEs long after the variable went out of scope. Perhaps a similar approach is useful here, too? Otherwise, using the DebugLoc is even stricter, so that's fine with me, too.

lib/CodeGen/LiveDebugVariables.cpp
1039 ↗(On Diff #116504)

Please remove the findNextInsertLocation - we don't do/need this any more.

1058 ↗(On Diff #116504)

return std::next(I); ? (not sure if this is better :-)

1071 ↗(On Diff #116504)

.

Ka-Ka added a comment.Sep 26 2017, 5:10 AM
In D38229#880149, @rnk wrote:

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

I agree that we need to be careful not to introduce faulty debug info, but I don't really see how this patch could introduce inaccurate debug info at least not for the testcase you specified. The patch will only reinsert DBG_VALUES when redefining the vreg within the vregs liverange (indicated by the slot indexes). Note that the patch only try to reinsert all values that was originally found in the input before they were removed by the "Live DEBUG_VALUE analysis" pass. No hoisting of DBG_VALUEs are done. As the liverange of the physical registers are changed to not be extended in the patch in D38140 (a prerequisite to this patch) no extra DEBUG_VALUEs will be inserted for physical registers.

When I tried your test program above I found that faulty hoisting of a DBG_VALUE across the call to usevals seems to be done by the CodeGenPrepare pass (before instruction selection). This must be a bug. If you want I can convert code above into a testcase but it will contain this faulty hoisted DBG_VALUE from the CodeGenPrepare pass. This patch will not impact that testcase.

Ka-Ka updated this revision to Diff 116793.Sep 27 2017, 4:24 AM
Ka-Ka marked 3 inline comments as done.

Minor fixes.

Ka-Ka added a comment.Sep 27 2017, 4:29 AM

When I tried your test program above I found that faulty hoisting of a DBG_VALUE across the call to usevals seems to be done by the CodeGenPrepare pass (before instruction selection). This must be a bug. If you want I can convert code above into a testcase but it will contain this faulty hoisted DBG_VALUE from the CodeGenPrepare pass. This patch will not impact that testcase.

There already exist an old bug report regarding CodeGenPrepare moving llvm.dbg.value without proper analysis. I appended a new reproducer to pr31878.
https://bugs.llvm.org/show_bug.cgi?id=31878

Ka-Ka added a comment.Sep 29 2017, 2:16 AM
In D38229#880149, @rnk wrote:

One concern I have about this is that it may lead to inaccurate debug info when the value is computed long before its (dead) assignment. Consider this kind of program (untested, yet):

int v1 = getval();
int v2 = getval();
usevals(v1, v2);
v1 = v2;
if (v1) { /* empty after inlining, making v2 dead */ }

After optimization, v1 will probably use a CSR and v2 will use a volatile register clobbered by the usevals call. So, the dead v1 = v2 assignment will produce a DBG_VALUE of a physical register that has been clobbered. It would be innacurate to hoist the DBG_VALUE across the usevals call, since the value should still be the result of the first getval call, not the second.

One way to address this would be to avoid moving DBG_VALUEs across instructions with different DebugLoc lines.

Even if we decide that this issue is unimportant, we should probably write tests for it.

I guess this is the case that you have concern about. Note that this is not actually how the input mir to greedy look like when running the code through llc (due to pr31878). I have edited the mir before feeding it to greedy to get the case that I think you are concerned about.

bb.0.entry:
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !14
  dead %eax = MOV32r0 implicit-def dead %eflags, implicit-def %al, debug-location !14
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit killed %al, implicit-def %rsp, implicit-def %eax, debug-location !14
  ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !14
  %2 = COPY killed %eax, debug-location !14
  DBG_VALUE debug-use %2, debug-use _, !11, !DIExpression(), debug-location !15
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !16
  dead %eax = MOV32r0 implicit-def dead %eflags, implicit-def %al, debug-location !16
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit killed %al, implicit-def %rsp, implicit-def %eax, debug-location !16
  ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !16
  %3 = COPY killed %eax, debug-location !16
  DBG_VALUE debug-use %3, debug-use _, !13, !DIExpression(), debug-location !17
  ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !18
  %edi = COPY %2, debug-location !18
  %esi = COPY %3, debug-location !18
  CALL64pcrel32 @usevals, csr_64, implicit %rsp, implicit killed %edi, implicit killed %esi, implicit-def %rsp, debug-location !18
  DBG_VALUE debug-use %3, debug-use _, !11, !DIExpression(), debug-location !15
  ADJCALLSTACKUP64 0, 0, implicit-def dead %rsp, implicit-def dead %eflags, implicit %rsp, debug-location !18
  RET 0, debug-location !19

When looking at the output after livedebugvariables it look like this:

bb.0.entry:
  liveins: %rbx

  frame-setup PUSH64r killed %rbx, implicit-def %rsp, implicit %rsp, debug-location !14
  CFI_INSTRUCTION def_cfa_offset 16
  CFI_INSTRUCTION offset %rbx, -16
  dead %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags, implicit-def %al, debug-location !14
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit %al, implicit-def %rsp, implicit-def %eax, debug-location !14
  %ebx = MOV32rr %eax, debug-location !14
  DBG_VALUE debug-use %ebx, debug-use _, !11, !DIExpression(), debug-location !15
  dead %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags, implicit-def %al, debug-location !16
  CALL64pcrel32 @getval, csr_64, implicit %rsp, implicit %al, implicit-def %rsp, implicit-def %eax, debug-location !16
  DBG_VALUE debug-use %eax, debug-use _, !13, !DIExpression(), debug-location !17
  %edi = MOV32rr killed %ebx, debug-location !18
  %esi = MOV32rr killed %eax, debug-location !18
  CALL64pcrel32 @usevals, csr_64, implicit %rsp, implicit killed %edi, implicit killed %esi, implicit-def %rsp, debug-location !18
  DBG_VALUE debug-use %eax, debug-use _, !11, !DIExpression(), debug-location !15
  %rbx = POP64r implicit-def %rsp, implicit %rsp, debug-location !19
  RETQ debug-location !19

The last DBG_VALUE is wrong:

DBG_VALUE debug-use %eax, debug-use _, !11, !DIExpression(), debug-location !15

As I understand your comment this is the situation you are worried about. The value in %eax do not contain any value connected to the variable the DBG_VALUE is suppose to describe as the range ended before the call to usevals. The register allocator might have reused eax for another value.

I agree this is a fault, but the output from livedebugvariables shown above is from what is already commited on trunk in svn. The output looks exactly the same with or without the patch in this review. Yes, this is a fault but it is another fault not related to this patch.

Could you please file a PR about this?

Could you please file a PR about this?

Done
https://bugs.llvm.org/show_bug.cgi?id=34788

Ka-Ka updated this revision to Diff 117657.Oct 4 2017, 3:58 AM

Rebased

rnk accepted this revision.Oct 4 2017, 9:21 AM

lgtm, my concern about hoisting can definitely be handled separately.

lib/CodeGen/LiveDebugVariables.cpp
1116 ↗(On Diff #117657)

These slot indices never cross BBs, right? We should be pre-layout here, so it wouldn't make sense to try to coalesce variable locations across MBBs.

test/DebugInfo/X86/pr34545.ll
26 ↗(On Diff #117657)

You know, we really should create an LLVM intrinsic for this. I would like a target-neutral way to emit a full register mask.

This revision is now accepted and ready to land.Oct 4 2017, 9:21 AM
This revision was automatically updated to reflect the committed changes.