[PATCH] D48968: [DebugInfo][LoopVectorize] Preserve DL in induction PHI and Add

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1824
                                     &*LoopVectorBody->getFirstInsertionPt());
+  VecInd->setDebugLoc(EntryVal->getDebugLoc());
   Instruction *LastInduction = VecInd;
----------------
gramanas wrote:
> vsk wrote:
> > Why is the debug location of the entry value the correct location to use for an induction variable phi? I'd expect a merged location to be applied to the phi, based on its incoming values.
> I figured that since this Phi is basically the for loop we are optimizing (in the test case below)
> `%vec.ind = phi <4 x i8> [ undef, %vector.ph ], [ %vec.ind.next, %vector.body ]` 
> it should get the same DL as the add instruction later. 
> 
> Also the entry val as mentioned in `InnerLoopVectorizer::widenIntOrFpInduction` (that calls the function I edited) is 
> `// The value from the original loop to which we are mapping the new induction variable.` 
> that I believe holds the correct DL.
> 
> OTOH, I could merge this DL with the vector preHeader BB's terminating instruction that results in this phi. Would this be a better approach?
> Basically I could add `setDebugLoc(DILocation::getMergedLocation(EntryVal->getDL(), Br->getDL())` after the Phi's incoming values are added, at the end of the function. 
Thanks for the explanation. Using EntryVal's location makes sense to me now.


================
Comment at: test/Transforms/LoopVectorize/i8-induction.ll:15
+;
+; DEBUGLOC:         ![[DbgLoc]] = !DILocation(line: 5
 scalar.ph:
----------------
gramanas wrote:
> vsk wrote:
> > vsk wrote:
> > > A reader might not expect to see a DILocation here. Please move this to the end of the file. That's where the metadata would appear in an IR dump.
> > Please add a comment explaining why the number '5' is here.
> I wanted to make sure that it's not an artificial DL. I made it explicit that it's not `line: 0` now.
I thought it was better to use line: 5, because it makes it clear that the location comes from "%c.015 = phi i8". A comment would help explain that to readers.


Repository:
  rL LLVM

https://reviews.llvm.org/D48968