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

[Anastasis via Phabricator via llvm-commits]

gramanas added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2887
 
+  // OldInduction could be nullptr. In that case we want the Debug Location
+  // from the old induction phi to be passed on to the new one.
----------------
vsk wrote:
> Please specify why OldInduction could be nullptr: there is no primary induction variable. In that case, what is the meaning of "the old induction phi"? Is there exactly one? If so, is it guaranteed to be in the original loop header?
The `LoopVectorizationLegality` class that determines what the PrimaryInduction variable will be. As I understand it, there are three cases:
 - There exists a primaryInduction
 - there might not be a PrimaryInduction at all
 - The widest instruction in the loop doesn't have the same type as the PrimaryInduction.

In the last two cases the PrimaryInduction will be `nullptr`.

The OldInduction in the `createVectroziedLoopSkeleton()` is this PrimaryInduction.

> is it guaranteed to be in the original loop header?

Since the function uses
```
BasicBlock *OldBasicBlock = OrigLoop->getHeader();
BasicBlock *VectorPH = OrigLoop->getLoopPreheader();
BasicBlock *ExitBlock = OrigLoop->getExitBlock();
``` 
I am not sure it could be anywhere else other than the OldBasicBlock.

---------------

>From what I gather if there is a PrimaryInduction the new Induction variable should have it's DL, I am not sure what should happen in the case there is no PrimaryInduction.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2898
+  } else {
+    InductionDL = getDebugLocFromInstOrOperands(OldInduction)->getDebugLoc();
+  }
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D49347