[PATCH] D153696: [LV] Only generate 1st part outside of vector region for VPInstruction.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1099
+bool VPValue::isDefinedOutsideVectorRegions() const {
+  return !hasDefiningRecipe() || !getDefiningRecipe()->getParent()->getParent();
+}
----------------
nit: this actually checks if a VPValue is "top level" - either live-in or defined outside any region. But non-vector regions currently reside inside vector regions, so this is equivalent.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:272
+  Value *get(VPValue *Def,
+             const std::variant<unsigned, VPIteration> &LaneOrIter);
 
----------------
Are these changes to the interface of State::get() still needed?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:296
+  void set(VPValue *Def, Value *V, unsigned Part);
+
   /// Reset an existing vector value for \p Def and a given \p Part.
----------------
Better keep code-gen and State::set() simple, modeling uniformity behavior explicitly and consistently in recipes, and only there?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:317
     } else {
       Next = State.get(this, 0);
     }
----------------
TODO: this replication should be taken out to the caller of generateInstruction for such VPInstructions known to be uniform but appearing inside the vector loop.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1184
   State.ExpandedSCEVs[Expr] = Res;
-  for (unsigned Part = 0, UF = State.UF; Part < UF; ++Part)
-    State.set(this, Res, {Part, 0});
+  State.set(this, Res, {0, 0});
 }
----------------
Let's leave it for a recipe generating an invariant/uniform to set all parts, for consistency, as in Replicate Recipe?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:192
   /// Returns true if the VPValue is defined outside any vector regions, i.e. it
   /// is a live-in value.
+  bool isDefinedOutsideVectorRegions() const;
----------------
The last i.e. part above is obsolete?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153696