[PATCH] D100260: [VPlan] Merge predicated-triangle regions, after sinking.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:111
+  for (VPBasicBlock *VPBB :
+       make_early_inc_range(VPBlockUtils::blocksOnly<VPBasicBlock>(Iter))) {
     for (auto &Recipe : *VPBB) {
----------------
Ayal wrote:
> Why make_early_inc_range here, we're traversing all VPBB's only to populate WorkList with VPValues.
I think that's a leftover from a previous rebase.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:153
+
+/// If \p R is a predicated triangle, return the 'then' block of the triangle.
+static std::pair<VPBasicBlock *, VPValue *>
----------------
Ayal wrote:
> Then perhaps "getPredicatedThenBlock(R)" would be a more accurate name than isPredicatedTriange()?
> 
> The first part checking if R's entry is a basic block with a single recipe being a Branch on Mask recipe, and returning its condition if so, can be done in a separate getPredicatedCondition(R)?
> 
> This would require later doing something like
> 
> ```
>   Cond1 = getPredicatedCondition(Region1);
>   Cond2 = getPredicatedCondition(Region2);
>   if (!Cond1 || Cond1 != Cond2)
>     continue;
>   Then1 = getPredicatedThenBlock(Region1);
>   Then2 = getPredicatedThenBlock(Region2);
>   if (!Then1 || !Then2)
>     continue;
> 
> ```
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:218
+    // the moment. The shuffled recurrence vector with all values needs to be
+    // generate before any users during code-gen currently.
+    auto IsImmovableRecipe = [](VPRecipeBase &R) {
----------------
Ayal wrote:
> Perhaps worth further clarifying: a recipe R feeding a first order recurrence phi must allow for a *vector* shuffle to be inserted immediately after it, and therefore if R is *scalarized and predicated* it must appear last in its basic block. In addition, other recipes may need to "sink after" R, so best if R not be moved at all.
> A word on not moving multi-def'd recipes, e.g., they are not expected, and to simplify looking after all users?
> Perhaps worth further clarifying: a recipe R feeding a first order recurrence phi must allow for a *vector* shuffle to be inserted immediately after it, and therefore if R is *scalarized and predicated* it must appear last in its basic block. In addition, other recipes may need to "sink after" R, so best if R not be moved at all.

Thanks, I extended the comment!

> A word on not moving multi-def'd recipes, e.g., they are not expected, and to simplify looking after all users?

There should be no multi-defs in a region, I turned it into an assert.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:253
+
+      ToMove.moveBefore(*Merge2, Merge2->begin());
+    }
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > moveBefore Merge2->begin() is right, because we're moving phi's here; worth a comment.
> > Yes. I updated the variable name to `PhiToMove`, so it should be clearer. I did not add a comment because it seems a bit redundant to me after the renaming. But I'd be happy to add one if you think it is still valuable after the renaming.
> +1
> 
> Another option may be "Phi1", as it is a Phi of Merge1, of Region1. All Phi1's are moved to appear right before Phi2's. Or "Phi1ToMove".
> 
> Perhaps "IncV" >> "PredInst1", given that it is the recipe in Then1 feeding [PredInst]Phi1/Phi1ToMove.
Added `1` to both names.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:245
+    delete Region;
+  }
+
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > delete DeletedBlocks?
> > moved the deletion outside to clean up all blocks in DeletedBlocks.
> Region1 itself should also be deleted, after its basic blocks are.
> But suffice to do DeletedRegions.insert(Region1); i.e., deleting every Region1 takes care to destroy all its basic blocks.
Good point, updated to just add `Region1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260