[PATCH] D69067: [LV] Record GEP widening decisions in recipe (NFCI)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This looks good to me, thanks, plus a couple of comments.

Worth noting that GEPs will now stop the extensibility of WidenRecipe's compaction optimization. But this compaction should eventually be removed to represent def-use relations.

Let's wait a few days if anyone else would like to review.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4016
+      unsigned I = 0;
+      for (auto &U : make_range(GEP->idx_begin(), GEP->idx_end())) {
+        if (IsIndexLoopInvariant[I++])
----------------
Can use GEP->indices() instead.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6873
   auto willWiden = [&](unsigned VF) -> bool {
     if (!isa<PHINode>(I) && (CM.isScalarAfterVectorization(I, VF) ||
                              CM.isProfitableToScalarize(I, VF)))
----------------
Checking if I isa PHINode here is completely redundant, given that all phi's were handled before tryToWiden(). But this cleanup calls for a separate patch.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6914
     LastExtensibleRecipe = WidenRecipe;
   setRecipe(I, WidenRecipe);
   VPBB->appendRecipe(WidenRecipe);
----------------
This setRecipe handles GEPs, and should continue to do so.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7012
+      return false;
+    VPBB->appendRecipe(new VPWidenGEPRecipe(GEP, OrigLoop));
+    return true;
----------------
Missing setRecipe(...) of the new recipe here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:683
+void VPWidenGEPRecipe::print(raw_ostream &O, const Twine &Indent) const {
+  O << " +\n" << Indent << "\"WIDEN-GEP " << VPlanIngredient(GEP) << "\\l\"";
+}
----------------
Can also print the in/variance information, although printing the GEP seems sufficient.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:767
+    unsigned I = 0;
+    for (auto &U : make_range(GEP->idx_begin(), GEP->idx_end()))
+      IsIndexLoopInvariant[I++] = OrigLoop->isLoopInvariant(U.get());
----------------
Can use GEP->indices() instead.


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

https://reviews.llvm.org/D69067