[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:8060
     if (CM.foldTailByMasking() && IndUpdate == Legal->getPrimaryInduction())
       continue;
 
----------------
Wonder if PrimaryInduction can have redundant casts; and if so, do they get removed now and/or with this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:307
+    auto &Casts = IV->getDescriptor().getCastInsts();
+    if (Casts.empty() || IV->getTruncInst())
+      continue;
----------------
If IV has a TruncInst all of its Casts are to be retained?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:310
+
+    // Visit all users of IV and replace cast instructions in Cast with IV and
+    // remember them for removal.
----------------
"in Cast[s]"


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115112