[PATCH] D142589: [LV] Perform recurrence sinking directly on VPlan.

Ayal Zaks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Feb 5 09:10:24 PST 2023


Ayal added a comment.

In D142589#4094683 <https://reviews.llvm.org/D142589#4094683>, @fhahn wrote:

> Address latest comments, thanks! I think it would be good to address the independent comments once the new sinking code has settled in.

Sure, also raises the thought of initially representing predicated regions as recipes when dealing with dominance and sinking/motion, then expand them to replicated regions before sinking scalars and code generation.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8975
 
-  // Introduce a recipe to combine the incoming and previous values of a
-  // fixed-order recurrence.
-  for (VPRecipeBase &R :
-       Plan->getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
-    auto *RecurPhi = dyn_cast<VPFirstOrderRecurrencePHIRecipe>(&R);
-    if (!RecurPhi)
-      continue;
-
-    VPRecipeBase *PrevRecipe = &RecurPhi->getBackedgeRecipe();
-    // Fixed-order recurrences do not contain cycles, so this loop is guaranteed
-    // to terminate.
-    while (auto *PrevPhi =
-               dyn_cast<VPFirstOrderRecurrencePHIRecipe>(PrevRecipe))
-      PrevRecipe = &PrevPhi->getBackedgeRecipe();
-    VPBasicBlock *InsertBlock = PrevRecipe->getParent();
-    auto *Region = GetReplicateRegion(PrevRecipe);
-    if (Region)
-      InsertBlock = dyn_cast<VPBasicBlock>(Region->getSingleSuccessor());
-    if (!InsertBlock) {
-      InsertBlock = new VPBasicBlock(Region->getName() + ".succ");
-      VPBlockUtils::insertBlockAfter(InsertBlock, Region);
-    }
-    if (Region || PrevRecipe->isPhi())
-      Builder.setInsertPoint(InsertBlock, InsertBlock->getFirstNonPhi());
-    else
-      Builder.setInsertPoint(InsertBlock, std::next(PrevRecipe->getIterator()));
-
-    auto *RecurSplice = cast<VPInstruction>(
-        Builder.createNaryOp(VPInstruction::FirstOrderRecurrenceSplice,
-                             {RecurPhi, RecurPhi->getBackedgeValue()}));
-
-    RecurPhi->replaceAllUsesWith(RecurSplice);
-    // Set the first operand of RecurSplice to RecurPhi again, after replacing
-    // all users.
-    RecurSplice->setOperand(0, RecurPhi);
-  }
+  VPlanTransforms::fixFirstOrderRecurrences(*Plan, Builder);
 
----------------
fhahn wrote:
> Ayal wrote:
> > nit: FOR now stands for FixedOrderRecurrence which leads to fixFixedOrderRecurrences() ... how about addSplicesToFixedOrderRecurrences()?
> Updated to `adjustFixedOrderRecurrences`, as it also requires sinking. WDYT?
Very good. A comment may help explain what `adjust` stands for.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:562
+    llvm_unreachable("recipe not found");
+  };
+  if (A->getParent() == B->getParent())
----------------
How about something like:

```
auto *ParentA = A->getParent();
auto *ParentB = B->getParent();
if (ParentA == ParentB)
  return LocalComesBefore(A, B);

auto *RegionA = GetReplicateRegion(const_cast<VPRecipeBase *>(A));
auto *RegionB = GetReplicateRegion(const_cast<VPRecipeBase *>(B));
if (RegionA)
  ParentA = RegionA->getExiting();
if (RegionB)
  ParentB = RegionB->getExiting();
return VPDT.dominates(ParentA, ParentB);
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:578
+  Seen.insert(Previous);
+  SmallVector<VPRecipeBase *> RecipesToSink;
+  auto TryToPushSinkCandidate = [&](VPRecipeBase *SinkCandidate) {
----------------
`RecipesToSink` should be defined next to `WorkList`, or folded to reuse `WorkList` or `Seen` instead: `RecipesToSink` is a copy of `WorkList` excluding initial FOR and all VPPredInstPHIRecipes - these can alternatively be ignored when traversing RecipesToSink. See below regarding using Seen instead of `RecipesToSink`.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:607
+  // processed first.
+  sort(RecipesToSink, [&VPDT](const VPRecipeBase *A, const VPRecipeBase *B) {
+    auto *RegionA = GetReplicateRegion(const_cast<VPRecipeBase *>(A));
----------------
Recipes to sink are already held in a set: can `Seen` be set to use this order and serve as RecipesToSink?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:611
+    if (RegionA && RegionB && RegionA != RegionB)
+      return VPDT.dominates(RegionA->getExiting(), RegionB->getExiting());
+
----------------
What if RegionA but not RegionB or vise versa? Should fold the replicate region logic into dominates() above?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:15
 #include "VPlanTransforms.h"
+#include "VPRecipeBuilder.h"
 #include "VPlanCFG.h"
----------------
fhahn wrote:
> Ayal wrote:
> > nit: lex order?
> moved, I am not sure why clang-format didn't fix it
still one out of place?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142589



More information about the llvm-commits mailing list