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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:321
+            Casts[NumCastsToRemove - 1] ==
+                UserCast->getVPSingleValue()->getUnderlyingValue()) {
+          FoundUserCast = UserCast;
----------------
Ayal wrote:
> 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])
> 
> ```
Thanks, reordered conditions.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:315
+    unsigned NumDeadCasts = 0;
+    while (NumDeadCasts != Casts.size()) {
+      assert(!IVUsers.empty() &&
----------------
Ayal wrote:
> 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.
> 
> 
I updated the code to iterate over the `Casts` vector instead of counting the casts we removed.

I also `CastsToRemove` to use `SmallVector<std::pair<VPRecipeBase *, VPValue *>>` and got back to the simpler iterating over phis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112