[PATCH] D68831: [LV] Mark instructions with loop invariant arguments as uniform. (WIP)

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for coming back to look into this @fhahn !

Overall, wonder what the current forward-propagating invariance analysis is missing, and if it's better to keep it separate from the backward-propagating "DemandedLanes" analysis aka uniform-after-vectorization. More specific comments inline.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2065
 
-  assert(Instance.Lane > 0
-             ? !Cost->isUniformAfterVectorization(cast<Instruction>(V), VF)
-             : true && "Uniform values only have lane zero");
-
+  // Always use lane 0 for uniform values.
+  unsigned Lane = Instance.Lane;
----------------
There's a distinction between (1) the truly "uniform values" of Legal->isUniform(), and (2) those of Cost->isUniformAfterVectorization() that "become" uniform. In both cases generating the single scalar value of lane zero suffices; the values of all other lanes need not be generated, because (1) they are all equal to that of lane zero, or (2) they are all dead. I.e., DemandedLanes={0} in case (2).

Instructions of case (1) should have ideally been LICM'd out, except for conditional instructions that have side-effects. Case (2) is what this assert is trying to verify - a user requesting the value of some lane>0 contradicts the liveness assumption; feeding it with the value of lane 0 may be feeding it a different, wrong value.

Note that this change to getOrCreateScalarValue() *alone* "fixes" PR40816, but there the users requesting the values of lanes>0 are essentially dead. Would be good to devise a test involving a predicated uniform-after-vectorization instruction, that is essentially live.

Would be good to come up with a better name than UniformAfterVectorization, which looks up a set named "Uniforms". Suggestions?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4703
+      // Instructions with loop invariant operands are uniform, as long as
+      // they do not read or write memory, are PHI nodes or terminators.
+      if (&I != BB->getTerminator() && !I.mayReadOrWriteMemory() &&
----------------
Such instructions should be identified as Legal->isUniform(), right?

Note that in general loop invariance is stronger than uniformity, though Legal->isUniform()  currently does return isLoopInvariant().


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4713
       // If there's no pointer operand, there's nothing to do.
       auto *Ptr = dyn_cast_or_null<Instruction>(getLoadStorePointerOperand(&I));
       if (!Ptr)
----------------
While we're here, _or_null should be dropped, &I cannot possibly be null.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5485
-  assert(!isUniformAfterVectorization(PredInst, VF) &&
-         "Instruction marked uniform-after-vectorization will be predicated");
 
----------------
Predicated instructions must not be uniform-after-vectorization currently, and this should better be checked earlier to avoid inserting them into Uniforms, or bailout from vectorizing, to prevent crashing as in  PR40816. This assert doesn't trigger there because it's masked under UseEmulatedMaskedMemRefHack().

When predicated instructions are allowed to be uniform-after-vectorization, it would be good for VPlan to reflect it, and have the VPRegionBlock built for it generate only a single replica (per part?) - that of lane zero.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5533
         if (isUniformAfterVectorization(J, VF))
           return false;
 
----------------
Comments and code above should be updated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68831