[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:321
+            Casts[NumCastsToRemove - 1] ==
+                UserCast->getVPSingleValue()->getUnderlyingValue()) {
+          FoundUserCast = UserCast;
----------------
Nit: the following reverse order more clearly coveys that checking a single defined value guards getVPSingleValue:

```
        if (UserCast->getNumDefinedValues() == 1 &&
            UserCast->getVPSingleValue()->getUnderlyingValue()) ==
                Casts[NumCastsToRemove - 1])

```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:315
+    unsigned NumDeadCasts = 0;
+    while (NumDeadCasts != Casts.size()) {
+      assert(!IVUsers.empty() &&
----------------
fhahn wrote:
> Ayal wrote:
> > This is fine, thanks!
> > 
> > Nits:
> > "while" >> "for", even if the bump is left inside?
> > In general better pre-compute Casts.size(), but loop is expected to be very short.
> > Another way of doing this, anticipating/enforcing a chain of casts:
> > 
> > 
> > ```
> >   VPRecipeBase *FindMyCast = IV;
> >   for (unsigned NumCastsToRemove =  Casts.size(); NumCastsToRemove > 0; NumCastsToRemove--) {
> >     VPRecipeBase *FoundUserCast = nullptr;
> >     for (auto &UserCast : FindMyCast->Users)
> >       if (UserCast.getNumDefinedValues() == 1 &&
> >           is_contained(Casts, UserCast.getVPSingleValue()->getUnderlyingValue())) {
> >         FoundUserCast = &UserCast
> >         break;
> >       }
> >     assert(FoundUserCast && "Missing a cast to remove");
> >     FoundUserCast->getVPSingleValue()->replaceAllUsesWith(IV);
> >     DeadCasts.push_back(FoundUserCast);
> >     FindMyCast = FoundUserCast;
> >   }
> > ```
> Thanks, I updated the code. With that style, we cannot RAUW in the loop, because this will nuke the users of `FoundUserCast`. I slightly adjusted the code so that DeadCasts also RAUW. Note that this required to adjust main loop a bit, now using the number of phi recipes. The phis() range relies in the first-non-phi iterator, which may be invalidated by removing recipes.
> 
> There's also no need to use `is_contained`. The instructions in `Casts` are in reverse visitation order and we can use that.
> There's also no need to use is_contained. The instructions in Casts are in reverse visitation order and we can use that.
In this case, how about iterating over Casts in reverse order, searching for the recipe of each Cast (instead of recording and retrieving it directly as raised earlier).

If DeadCasts ("CastsToRemove"?) accumulates all candidate recipes and these are removed in a separate loop at the end, the main loop can more simply iterate over phis. The RAUW loop can loop over the last Casts.size() entries of DeadCasts (in reverse? ;-), or a separate CastsToRAUW can be used which can fill CastsToRemove; or a <Recipe, IV> map can be used to also delay RAUW to the end.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112