[PATCH] D142886: [VPlan] Switch to checking sinking legality for recurrences in VPlan.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:189
   /// the iteration before the previous, we refer to it as second-order
-  /// recurrence and so on). \p SinkAfter includes pairs of instructions where
-  /// the first will be rescheduled to appear after the second if/when the loop
-  /// is vectorized. It may be augmented with additional pairs if needed in
-  /// order to handle Phi as a first-order recurrence.
-  static bool
-  isFixedOrderRecurrence(PHINode *Phi, Loop *TheLoop,
-                         MapVector<Instruction *, Instruction *> &SinkAfter,
-                         DominatorTree *DT);
+  /// recurrence and so on).
+  static bool isFixedOrderRecurrence(PHINode *Phi, Loop *TheLoop,
----------------
Ayal wrote:
> Leave a comment that this optimistically assumes instructions may be reordered successfully if needed?
Added, thanks!


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:967
 
-  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous) ||
-      SinkAfter.count(Previous)) // Cannot rely on dominance due to motion.
+  if (!Previous || !TheLoop->contains(Previous) || isa<PHINode>(Previous))
     return false;
----------------
Ayal wrote:
> (Relying here on Previous is fragile as it may have been considered for sinking.)
Agreed, but I think until we fully check legality in VPlan I think it is needed for now here (and I think it is not different to the current handling)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:651
   const VPRegionBlock *RegionB =
       GetReplicateRegion(const_cast<VPRecipeBase *>(B));
   if (RegionA)
----------------
Ayal wrote:
> nit: is this consideration of replicate regions is still needed here, or can be replaced by an assert?
Thanks, adjusted in c18bc7f7feac.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:689
     VPRecipeBase *Current = WorkList[I];
-    assert(Current->getNumDefinedValues() == 1 &&
-           "only recipes with a single defined value expected");
-    for (VPUser *User : Current->getVPSingleValue()->users()) {
-      if (auto *R = dyn_cast<VPRecipeBase>(User))
-        TryToPushSinkCandidate(R);
+    for (VPValue *Val : Current->definedValues()) {
+      for (VPUser *User : Val->users()) {
----------------
Ayal wrote:
> Is this change to support multiple def'd recipes independent of the rest of this patch?
Yep that's independent of the patch, I restored the original code here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:740
+      return false;
+    SeenPrevious.insert(Previous);
 
----------------
Ayal wrote:
> Is `SeenPrevious` really needed, to make sure chained FORs are processed in order, or can it be removed - given that every FOR takes care of its own sinkings followed by introducing its splice hooking it up to defs and uses? Potentially sinking candidates multiple times.
I checked and it is not needed in the latest version indeed! It also yields a nice improvement in `@test_pr54223_sink_after_insertion_order`! Updated to remove it.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:79
   /// current implementation assumes all users can be sunk after the previous
   /// value, which is enforced by earlier legality checks.
+  static bool adjustFixedOrderRecurrences(VPlan &Plan, VPBuilder &Builder);
----------------
Ayal wrote:
> Explain return value?
Added a comment, thanks!


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll:637
 
-; Make sure LLVM doesn't generate wrong data in SinkAfter, and causes crash in
-; loop vectorizer.
 define void @test_crash(ptr %p) {
+; CHECK-LABEL: @test_crash(
----------------
Ayal wrote:
> nit: update name of test, and its comment?
Added, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142886