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

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 16 12:34:55 PDT 2018


vsk added a comment.

Thanks! Please add a few reviewers who are actively working on LV.

Consider retitling this to include '[LV]' instead of '[InstCombine]'? More comments inline --



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2572
+  DebugLoc Loc;
+  DL ? Loc = DL->getDebugLoc() : Loc = L->getStartLoc();
+
----------------
gramanas wrote:
> gramanas wrote:
> > This is redundant as the variable DL has the exact same pointer as OldInst, that gets it from the call to `createInductionVariable`
> `L->getStartLoc()` doesn't seem to be the correct debug loc for the phi and add instructions below. I've been using the same test as rL336667 for the tests and I think the correct DL to attach to these instructions is the one from `%mul16 = phi i8`.
> 
> @vsk what do you think?
Hm, it makes sense that the starting location of the loop doesn't match the location of one of its IVs. As for which location is correct, see my other comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:470
   PHINode *createInductionVariable(Loop *L, Value *Start, Value *End,
-                                   Value *Step, Instruction *DL);
+                                   Value *Step, DebugLoc *OldInductionDL);
 
----------------
DebugLoc can be passed by value cheaply. You don't need a pointer here.


================
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.
----------------
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?


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


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2899
+    InductionDL = getDebugLocFromInstOrOperands(OldInduction)->getDebugLoc();
+  }
+
----------------
Please follow the style and structure of the surrounding code. Extract the logic to find the right location into a static helper.


Repository:
  rL LLVM

https://reviews.llvm.org/D49347





More information about the llvm-commits mailing list