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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3714
-
-    fixLCSSAPHIs(State);
   }
----------------
Ayal wrote:
> OK, this is where LCSSA phis were fixed originally, can clear LiveOuts here if requiresScalarEpilogue().
Sounds good, that's what the latest version of the patch is doing.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3870
+        State.Plan->removeLiveOut(&LCSSAPhi);
+      }
 }
----------------
Ayal wrote:
> 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
I think for now it is easiest to keep the check and remove it once we migrate exit value handling for those.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8717
+  // Only handle single-exit loops for now.
+  if (!ExitBB->getSinglePredecessor())
+    return;
----------------
Ayal wrote:
> 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?
I'll adjust the comment here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537