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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:562
+    llvm_unreachable("recipe not found");
+  };
+  if (A->getParent() == B->getParent())
----------------
Ayal wrote:
> 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);
> ```
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:578
+  Seen.insert(Previous);
+  SmallVector<VPRecipeBase *> RecipesToSink;
+  auto TryToPushSinkCandidate = [&](VPRecipeBase *SinkCandidate) {
----------------
Ayal wrote:
> `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`.
> 
Yeah, updated to ignore the reciepes instead and used WorkList.


================
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));
----------------
Ayal wrote:
> Recipes to sink are already held in a set: can `Seen` be set to use this order and serve as RecipesToSink?
SmallPtrSet and SetVector cannot be sorted unfortunately. Updated to use WorkList.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142589