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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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(
----------------
Ayal wrote:
> Worth retaining some of this explanation, somewhere?
I tried to capture the essence (the bit about why the casts are redundant) in the doc-comment for `removeRedundantInductionCasts`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9105
+      auto *CastDef = R->getVPSingleValue();
+      if (!is_contained(Casts, CastDef->getUnderlyingValue()))
+        continue;
----------------
Ayal wrote:
> 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?
> Would be good to ensure that all members of Casts get processed.

I added an assert that we add exactly `Casts.size()` elements to remove.

> How about recording the recipes of Casts upfront, and traverse Casts here retrieving their recipes directly and RAUW them with IV?

I moved it to `VPlanTransforms`, but retained the logic to discover the casts be traversing IV users. I am not sure if it is worth relying on the extra state in this case, we can get all the information directly from the plan without too much effort. We only need to look through instructions in `Casts`, so it should be a very small amount of extra work. WDYT?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9113
+  for (VPRecipeBase *R : DeadCasts)
+    R->eraseFromParent();
+
----------------
Ayal wrote:
> 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.
There's no VPlan DCE yet unfortunately, so I kept it for now. But that's certainly something to follow up on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112