[PATCH] D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 5 inline comments as done.
jmorse added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:6931
       // where said address is used.
       if (!DVI || (DVI->getValue() && isa<AllocaInst>(DVI->getValue()))) {
         PrevNonDbgInst = Insn;
----------------
bjope wrote:
> I think that the special handling of alloca can be removed now (either the alloca dominates the DVI and everyhing is fine, or is actually is possible that the DVI dominates the alloca, then I think we can move it).
> Although, no need to make such a change in this patch if you think it is risky/unrelated.
I agree and have removed it -- it's redundant and misleading to a future reader to be calculating the "previous non debug instruction" when there's a dominator tree check happening.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:6937
       Instruction *VI = dyn_cast_or_null<Instruction>(DVI->getValue());
+      if (VI && DT.dominates(VI, DVI))
+        continue;
----------------
bjope wrote:
> aprantl wrote:
> > Please add a comment that explains why
> I think this explanation can be added by updating the description of this function. Mentioning that the purpose is to resolve use-before-def problems (which really should be seen as bugs in this, as well as other, passes):
>   "This function only moves (sinks) DbgValueInst:s when there is a use-before-def situation."
Updated the function comment; I threw a "FIXME" in too indicating that placeDbgValues is a concession, rather than design feature.


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

https://reviews.llvm.org/D58453