[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/LoopVectorize.cpp:9105
+      auto *CastDef = R->getVPSingleValue();
+      if (!is_contained(Casts, CastDef->getUnderlyingValue()))
+        continue;
----------------
fhahn wrote:
> 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?
> 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?

Doing

```
  for (auto &Cast : Casts)
    RecipeBuilder.getRecipe(Cast)->getVPSingleValue()->replaceAllUsesWith(IV);
```
does seem (more) straightforward, albeit dependent on recording Casts in RecipeBuilder, and does not convey nor check that Casts are chained from IV.
Amount of extra work is indeed expected to be small, and can be slightly decreased - see below. Furthermore, perhaps at some point this could become a truly VPlan-to-VPlan transformation, which also identifies the redundancy of such casts independently, instead of relying on initial external identification. Ship it!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:310
+
+    unsigned NumDeadCastsAtStart = DeadCasts.size();
+    // Visit all users of IV and replace cast instructions in Cast with IV and
----------------
Used only in assert, needs #ifndef?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:315
+    IVUsers.append(IV->user_begin(), IV->user_end());
+    while (!IVUsers.empty()) {
+      VPRecipeBase *R = cast<VPRecipeBase>(IVUsers.pop_back_val());
----------------
(This cannot loop forever because Casts excludes the header phi chaining Casts into a cycle.)

There typically will be a few Casts if any, but it does seem somewhat wasteful to go through all users of all Casts. Perhaps loop until the last Cast is reached, saving the redundant traversal over its users? (And reducing the risk of infinite loop if something went wrong?)
Can also repeatedly search for a single next cast among the users of the current CastDef, stopping as soon as one is found, instead of always appending all users for traversal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112