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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Marvelous!
Worth adding in the summary that all redundant casts will now first be represented in VPlan using recipes, instead of being treated as DeadInstructions and rejected out of hand.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:659
-  /// that IV, but it's error-prone to expect callers of this routine to care
-  /// about that, hence this explicit parameter.
-  void recordVectorLoopValueForInductionCast(
----------------
Worth retaining some of this explanation, somewhere?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9088
 
+  // Remove dead casts of induction.
+  SetVector<VPRecipeBase *> DeadCasts;
----------------
`dead` >> `redundant`?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9105
+      auto *CastDef = R->getVPSingleValue();
+      if (!is_contained(Casts, CastDef->getUnderlyingValue()))
+        continue;
----------------
Would be good to ensure that all members of Casts get processed.
How about recording the recipes of Casts upfront, and traverse Casts here retrieving their recipes directly and RAUW them with IV?

Note that this Redundant Cast Elimination can-be/is done before sinkAfter, because such casts can feed only themselves, so they cannot feed candidates that want to sinkAfter them. However, perhaps better to swap them, given that sinkAfter is mandatory whereas RCE is an optimization - presumably code is functionally correct (yet suboptimal) w/o it? Worth outlining to reduce the size of buildVPlanWithVPRecipes() which is already excessive?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9113
+  for (VPRecipeBase *R : DeadCasts)
+    R->eraseFromParent();
+
----------------
Now that the redundant casts became dead - useless, they can be cleaned up with a general VPlan DCE, which may become useful in additional cases. This local self-clean-up is also fine of-course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112