[PATCH] D118051: [VPlan] Remove dead header-phi recipes.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:303
 
-    // Visit all casts connected to IV and in Casts. Collect them.
-    // remember them for removal.
+    // A sequence of IR Casts has potentially been recorded for IV, which need
+    // to be bypassed when the IV is vectorized, because the vectorized IV will
----------------
Ayal wrote:
> Worth emphasizing "need to be" - unlike other VPlanTransforms/VPlan2VPlan optimizations, this optimization is mandatory.
> Perhaps better call it "removeObsoleteInductionCasts()"? (comment is indep. of this patch)
> Worth emphasizing "need to be" - unlike other VPlanTransforms/VPlan2VPlan optimizations, this optimization is mandatory.

I updated the section to use `*must ...*` to emphasize it.

> Perhaps better call it "removeObsoleteInductionCasts()"? (comment is indep. of this patch)

Will do independent of this patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:367
+  // TODO: Remove dead recipes across whole plan.
+  for (VPRecipeBase &R : make_early_inc_range(reverse(*Header))) {
+    if (R.mayHaveSideEffects() ||
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > reverse is needed in order to collect dead chains/DAGs, right?
> > Yes, it allows us to catch chains of dead recipes.
> That may be worth pointing out.
Thanks, I added a sentence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118051