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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8060
     if (CM.foldTailByMasking() && IndUpdate == Legal->getPrimaryInduction())
       continue;
 
----------------
Ayal wrote:
> Wonder if PrimaryInduction can have redundant casts; and if so, do they get removed now and/or with this patch?
I tried to construct such a test case, but I don't think that's possible at the moment. We require the trip-count to be computable to start with, but when the primary induction variable has casts, I don't think that would be the case. At least I couldn't construct a test. In any case, I think it would probably crash with the code currently in tree and would be handled correctly with the patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:307
+    auto &Casts = IV->getDescriptor().getCastInsts();
+    if (Casts.empty() || IV->getTruncInst())
+      continue;
----------------
Ayal wrote:
> If IV has a TruncInst all of its Casts are to be retained?
For now, the TruncInst handling is completely separate tat the moment; If the Trunc has not been removed as dead, we will create a new (optimized) VPWidenIntOrFpInductionRecipe for the TruncInst, so there is nothing to remove here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:315
+    unsigned NumDeadCasts = 0;
+    while (NumDeadCasts != Casts.size()) {
+      assert(!IVUsers.empty() &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112