[PATCH] D88406: [LiveDebugValues][InstrRef][2/2] Emit entry value variable locations

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3116
+      // transfers after them.
+      if (P.Pos->isTerminator())
+        continue;
----------------
jmorse wrote:
> djtodoro wrote:
> > Should this be a separate patch? Does this affect/apply to the "copies" as backups?
> Shouldn't affect variable locations; I'll split it into a separate patch though and have a dedicated test case. The quick summary is that on X86 tailcalls are written:
> 
>     TAILCALL @foo, implicit $rdi, $implicit-def $rax
> 
> Or similar, and are the last instruction in the block. InstrRefBasedLDV sees the implicit-def of $rax, and tries to recover variable locations afterwards, that leads to DBG_VALUEs being inserted after the terminator; which the MachineVerifier complains about.
> 
> There may be some exotic architectures out there that can have multiple terminators in their block, and def registers on the first terminator, which would be tricker to handle. But I think that bridge can be crossed when we come to it.
OK. That is fine with me. We avoid such entry-values within old-(varloc-based)-ldv as well.

>There may be some exotic architectures out there that can have multiple terminators in their block, and def registers on the first terminator, which would be tricker to handle. But I think that bridge can be crossed when we come to it.

I think that I have faced such case on the PPC64, but as long as we have the `MachineVerifier` checking that, we are safe.


================
Comment at: llvm/test/DebugInfo/MIR/X86/livedebugvalues_load_in_loop.mir:83
     $rbp = MOV64rr $rdi, debug-location !17
+    DBG_VALUE $rbp, $noreg, !16, !DIExpression(), debug-location !17
     dead $rcx = MOV64ri 0, debug-location !17
----------------
jmorse wrote:
> djtodoro wrote:
> > Why including this here?
> /me squints; I think I moved this because otherwise the the output looks like this:
> 
>     DBG_VALUE $rdi
>     $rbp = MOV64rr $rdi
>     $rcx = MOV64ri 0
>     CALL64pcrel32 @bees
>     DBG_VALUE $rbp
> 
> i.e., the location is recovered to $rbp after the call clobbers $rdi. Wheras with this change, LiveDebugValues does not have to recover any clobbers.
> 
> My thinking was that, because this test is targeted at one thing VarLocBasedLDV does poorly, it's best to reduce the number of features being tested in the test, so that clobber being recovered from should be avoided.
OK then :)

Does moving it into the previous change(D88405) make sense to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88406