[PATCH] D115112: [LV] Remove dead IV casts using VPlan (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:325
+  for (auto &E : CastsToRemove) {
+    E.first->getVPSingleValue()->replaceAllUsesWith(E.second);
+    E.first->eraseFromParent();
----------------
Note that this order of traversal, going forward from the phi, is the more efficient one for RAUW'ing.
Going backwards towards the phi, following the original order of Casts, would result in repeated RAUW's.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:307
+    auto &Casts = IV->getDescriptor().getCastInsts();
+    if (Casts.empty() || IV->getTruncInst())
+      continue;
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > If IV has a TruncInst all of its Casts are to be retained?
> > > For now, the TruncInst handling is completely separate tat the moment; If the Trunc has not been removed as dead, we will create a new (optimized) VPWidenIntOrFpInductionRecipe for the TruncInst, so there is nothing to remove here.
> > It seems a bit more logical then to have "if (!IV || IV->getTruncInst()) continue", thereby indicating which recipes are (ir)relevant for this transformation.
> > 
> > If an IV with a TruncInst can have Casts, are their recipes removed/not-generated elsewhere? Otherwise, can we assert that there's no TruncInst after we checked that there are Casts?
> Thanks, the latest version indeed doesn't really need to check the size of `Casts` any more, because it directly iterates over them, so there's nothing extra to do it empty. Adjusted!
> there's nothing extra to do it empty. Adjusted!
Good!

> If the Trunc has not been removed as dead, we will create a new (optimized) VPWidenIntOrFpInductionRecipe for the TruncInst, so there is nothing to remove here.
ok, agreed. Would be good to add a line explaining why we must bail-out when IV has a Trunc. An IV phi feeding one or more Truncs will have multiple VPWidenIntOrFpInductionRecipes associated with it; its redundant casts however need to be removed exactly once - e.g., when reaching the recipe associated directly with it, having no Trunc, as done here.
(pr35773.ll demonstrates this by failing due to missing the casts to remove, if this bail-out is removed.)

Perhaps this Trunc-of-an-IV optimization should also be done as a VPlan-to-VPlan transformation, placed after Redundant Casts Elimination (which originated in D38948). It goes back to e266efb70bfdd which moved truncation of a vectorized IV to operate on the scalar IV.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112