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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:215
+    // merge block. Update all users inside the successor region to use the
+    // original values.
+    for (VPRecipeBase &ToMove : make_early_inc_range(reverse(*MergeRegion))) {
----------------
fhahn wrote:
> Ayal wrote:
> > Note: this takes care of fusion-admitting, lane-by-lane dependencies, right?
> Yes, it updates the uses outside the merged region.
> 
> Note: this takes care of fusion-admitting, lane-by-lane dependencies, right?

Other thing to consider are memory dependencies. But I don't think at the moment this would be an issue, because the case where we read from a location that is overwritten by a merged store should be rejected by the earlier legality checks. Otherwise we would also not be able to re-order the accesses for widening.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:216
+    // original values.
+    for (VPRecipeBase &ToMove : make_early_inc_range(reverse(*MergeRegion))) {
+      VPValue *IncV = cast<VPPredInstPHIRecipe>(&ToMove)->getOperand(0);
----------------
fhahn wrote:
> Ayal wrote:
> > Is the reverse required? Could repeatedly take (r)begin until empty.
> > 
> > Initially each MergeRegion has a single PredInstPHIRecipe, but multiple recipes would appear after merging, right? Worth testing merging three regions into one, if not done so already.
> I added a test `merge_3_replicate_region`, which exposed a problem with multiple `VPPredInstPHIRecipes` in the same block. I put up D104188 to fix that.
> Is the reverse required? Could repeatedly take (r)begin until empty.

moving them in reverse order seem natural (dominance between defs and uses is not broken temporarily) and `reverse()` should just reverse the iterators and not have any additional runtime cost.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:225
+            continue;
+          U->setOperand(I, IncV);
+        }
----------------
fhahn wrote:
> Ayal wrote:
> > `I` could appear as multiple operands of `U`, so avoid early-exit on first match, right? Perhaps worth a test.
> > 
> > Assert that at-least one such operand was found and (re)set?
> Yes, there could be multiple uses of  `ToMove` in `U`. I still need to add a test case for that.
Test is `@update_2_uses_in_same_recipe_in_merged_block`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:194
+
+    // If a recipe is used by a first-order recurrence phi, we cannot move it.
+    if (any_of(*ThenRegion, [](VPRecipeBase &R) {
----------------
Ayal wrote:
> because ... it may conflict with sink-after?
> 
> comment here should probably state what it is we can move.
> Could there potentially be fusion-preventing dependencies between the two regions (packing/unpacking?), or does the lack of recipes in the 'middle' block guarantee their absence?
The problem with first-order recurrences is that the during code-gen the recurrence vector for the current iteration must be generated *before* all FOR users and *after* all parts of the recurrence value have been computed.

Note that this depends on D100260 and I added an additional test to cover the problematic case `recipe_in_merge_candidate_used_by_first_order_recurrence`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260