[PATCH] D49347: [WIP][DebugInfo][LV] DebugLoc in induction PHI

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Can there be multiple induction phis in the original loop's header? If so, why is picking the location from the first phi correct?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2898
+  } else {
+    InductionDL = getDebugLocFromInstOrOperands(OldInduction)->getDebugLoc();
+  }
----------------
gramanas wrote:
> vsk wrote:
> > The idea behind 'getDebugLocFromInstOrOperands' seems unsound to me. An instruction and its operands aren't interchangeable in general, which is why it's not a good idea to pretend that their debug locations are interchangeable. We should take this opportunity to stop using this API.
> > We should take this opportunity to stop using this API
> 
> I agree, but simply using `OldInduction->getDebugLoc()` causes the `test/Transforms/LoopVectorize/debugloc.ll` to fail. After looking at it a bit more, it seems that this test is not particularly useful. The induction phi doesn't have debug location so the call to this function is justified by taking the DL from the `br` instruction.
> 
> Sadly there is no source available to recreate the test without the DI embedded. Do you think I should make a new one?
Is the test checking for a behavior we actually want? In this case I'd say probably not. The behavior is strange: we pick the location of a branch instruction by accident, and it's not clear that it would map back to the original source well.

I think it'd be appropriate to update or replace this test.




Repository:
  rL LLVM

https://reviews.llvm.org/D49347