[PATCH] D123537: [VPlan] Model first exit values using VPLiveOut.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3870
+        State.Plan->removeLiveOut(&LCSSAPhi);
+      }
 }
----------------
fhahn wrote:
> Ayal wrote:
> > Single else placed earlier suffices, no need for same else here as well?
> I think I might be missing something. Are you referring to removing all live outs earlier in `fixVectorizedLoop` via `Plan.clearLiveOuts`? This should only cover the case where we require scalar epilogues, whereas here we only deal with the other case.
Yes; we’re also checking if requiresScalarEpilogue() here, as done earlier and later when dealing with various reductions/inductions/recurrences


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4056
+        State.Plan->removeLiveOut(&LCSSAPhi);
+      }
 
----------------
fhahn wrote:
> Ayal wrote:
> > A bit confusing when to else cleanup LiveOuts; perhaps do so directly rather than as an else, or when creating LiveOuts?
> I'm not entirely sure what you are suggesting here. The reason this is done here to only remove the live-out of the current reduction. Doing so when creating LiveOuts is more complicated (as in earlier versions of the patch).
Was only referring to avoiding creating any LiveOut if requiresScalarEpilogue().


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8717
+  // Only handle single-exit loops for now.
+  if (!ExitBB->getSinglePredecessor())
+    return;
----------------
fhahn wrote:
> Ayal wrote:
> > Can fold both early exits into one.
> > 
> > Also early exit if requiresScalarEpilogue()?
> > Can fold both early exits into one.
>  
> Done, thanks!
> 
> > Also early exit if requiresScalarEpilogue()?
> 
> I think we should still create live-outs if `requiresScalarEpilogue`, otherwise some recipes may be considered dead if they only have uses outside the loop. The latest version of the patch uses that property to remove a workaround in VPlanTransforms that used IR def use chains to detect users outside the loop.
>> Also early exit if requiresScalarEpilogue()?
> I think we should still create live-outs if requiresScalarEpilogue, otherwise some recipes may be considered dead if they only have uses outside the loop. The latest version of the patch uses that property to remove a workaround in VPlanTransforms that used IR def use chains to detect users outside the loop.

This perhaps deserves a comment somewhere, explaining that VPLiveOuts are used to represent loop live out values that originally fed LCSSA phis in Exit Block, where these phis may or may not require fixing - if their edge is removed due to required scalar epilogue - in which case they are still live out but feed only preheader of scalar epilogue?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3714
-
-    fixLCSSAPHIs(State);
   }
----------------
OK, this is where LCSSA phis were fixed originally, can clear LiveOuts here if requiresScalarEpilogue().


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:386
-}
-
 void VPlanTransforms::removeDeadRecipes(VPlan &Plan, Loop &OrigLoop) {
----------------
 Nice cleanup!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537