[PATCH] D112549: Adding patch and unittest to generalize ignorable induction casts in the LoopVectorizationLegality analysis.

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:524
     InductionCastsToIgnore.insert(*Casts.begin());
+  for (Value *O : cast<User>(Phi)->users())
+    if (auto *I = dyn_cast<Instruction>(O))
----------------
Isn't the subject cast available in the list returned by getCastInsts()?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:528
+        if (auto *Cast = dyn_cast<CastInst>(I))
+          if (Cast->getSrcTy() == PhiTy) {
+            InductionCastsToIgnore.insert(Cast);
----------------
Checking the source type of the cast to match the phi type seems redundant given that we begin by identifying casts that are users of the phi. 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:529
+          if (Cast->getSrcTy() == PhiTy) {
+            InductionCastsToIgnore.insert(Cast);
+          }
----------------
The comment in LoopVectorizationLegality.h indicates that this list is supposed to contain casts that are noop (possibly guaranteed by some runtime check). IIUC your use case may not have that guarantee. Looking through the code though, it seems we only use this list in cost-estimation, so perhaps it's fine...but if that's the case we should update the comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112549/new/

https://reviews.llvm.org/D112549