[PATCH] D144491: [VPlan] Use isUniformAfterVec in VPReplicateRecipe::execute.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

LGTM, with a couple of nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9609
 
-  // A store of a loop varying value to a loop invariant address only
-  // needs only the last copy of the store.
-  if (isa<StoreInst>(UI) && !getOperand(1)->hasDefiningRecipe()) {
+  // A store of a loop varying value to a uniform address only needs only the
+  // last copy of the store.
----------------
Ayal wrote:
> only needs a single only...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9612
+  if (isa<StoreInst>(UI) &&
+      vputils::isUniformAfterVectorization(getOperand(1))) {
     auto Lane = VPLane::getLastLaneForVF(State.VF);
----------------
Ayal wrote:
> This is a bit fragile considering that isUniformAfterVectorization() in general also represents lane-varying values whose first lane need only be used, as in vector stores - see `isUniformDecision `.
This patch is fine - effectively considering Replicate recipe invariants as "uniform addresses" in addition to current isLiveIns (aka !hasDefiningRecipe).

Above comment (independent of this patch) refers to vputils which introduced "onlyFirstLaneUsed" to handle the "AfterVectorization" part, and its isUniformAfterVectorization() should be renamed isUniform(). In fact, it currently checks isInvariant(), i.e., same value across all loop iterations. This could be improved to "uniform" - same value across lanes (per part) as in D148841, and possibly also to same value across lanes*parts (per vectorized iteration) - but the latter refers to UF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144491