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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks for taking care of this, Andrei. Some comments below.



================
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).
----------------
The previous documentation seems clearer to me but I could be missing some subtle detail.
Typo: remove ')'?


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


================
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
----------------
Move comment inside the 'if'?


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/pr36524.ll:7
+; CHECK-LABEL: @foo(
+; CHECK:       vector.body:
+; CHECK-NEXT:    [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH:%.*]] ], [ [[INDEX_NEXT:%.*]], [[VECTOR_BODY:%.*]] ]
----------------
Is it necessary to check all the instructions in the loop body? Maybe we could remove some of them (e.g., icmp, br, ...) and strictly check whether the loop has been vectorized (vector.body) and the IV related instructions? Otherwise, this test might fail in the future due to changes unrelated to this fix.


https://reviews.llvm.org/D43812