[PATCH] D43812: [LV] Let recordVectorLoopValueForInductionCast to check if IV was created from the cast.

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:573
   /// \p EntryVal is the value from the original loop that maps to the steps.
-  /// Note that \p EntryVal doesn't have to be an induction variable (e.g., it
-  /// can be a truncate instruction).
-  void buildScalarSteps(Value *ScalarIV, Value *Step, Value *EntryVal,
+  /// Note that \p EntryVal doesn't have to be an induction variable - it
+  /// can also be a truncate instruction).
----------------
a.elovikov wrote:
> dcaballe wrote:
> > The previous documentation seems clearer to me but I could be missing some subtle detail.
> > Typo: remove ')'?
> The reason I made this change is because all the code I've seen expects EntryVal to be either PHI or TruncInst without any other possibilities. It is also true in the places where the parameters to the calls to this routine are formed. I've added an assert to ensure this property won't break - but that also required this small correction to the doc string.
> 
> So the change in a few words: before - phi/trunc/anything else is possible, after: only phi/trunc are expected.
> 
> Thanks for noticing ")"!
Ok, thanks for the clarification.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:605
+  /// phi node and is used to distinguish what is the IV currently being
+  /// processed - original one or the "newly-created" one based on the proof
+  /// mentioned above. We must not record anything for the latter case, but it's
----------------
a.elovikov wrote:
> dcaballe wrote:
> > It seems there is some given assumption that is not totally clear. The previous comments don't explicitly say anything about newly-created IVs. However, it seems that depending on the kind of EntryVal, we are able to know if the IV is an original or a newly-created one. Could you, please, elaborate the documentation a bit more in this regard and explain which kinds (Trunc) mean newly-created IV and why?
> These assumptions come from the callers of this routine, namely buildScalarSteps/createVectorIntOrFPInductionPHI.
> 
> This routine only makes sense in that context (and that was before my change), so I don't think we need to document the real meaning of the EntryVal here - only how it affects the processing being done in this routine.
I think that a quick comment would be necessary, at least, to be more explicit on where to find further information. Something like what you just said: "This function is used in the context of 'buildScalarSteps' and 'createVectorIntOrFPInductionPHI', where X and Y is assumed."


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2501
+
+  // This induction variable is not the phi from the original loop but the
+  // newly-created IV based on the proof that casted Phi is equal to the
----------------
a.elovikov wrote:
> dcaballe wrote:
> > Move comment inside the 'if'?
> I briefly looked through this file, it seems more common to have the comment the way it is now. See line 2610 as an example (one-line if condition with the preceding comment).
I've been asked to do the same change in this file. I think it makes sense since you are actually describing what happens in the if-then branch. Someone without enough knowledge of this code might understand that the comment is a generic description. If you move it inside the 'if', there is no room for doubt. But this is just my suggestion.


https://reviews.llvm.org/D43812