[PATCH] D149893: Rewrite LSV to handle longer chains.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:178-179
+  for (const auto &E : C) {
+    dbgs() << "  " << *E.Inst << " (offset " << E.OffsetFromLeader << ")"
+           << "\n";
+  }
----------------
Can combine the last two printed strings 


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:251
+  // done with the BB and erase all at once.
+  SmallVector<Instruction *, 16> ToErase;
+
----------------
Seems like a small size to use. I'd expect the number to be much larger in most cases 


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:418
   bool Changed = false;
+  LLVM_DEBUG(dbgs() << "LSV: Running on function " << F.getName() << "\n");
 
----------------
I find such printing redundant with regular pass manager debugging 


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:435
   for (BasicBlock *BB : post_order(&F)) {
-    InstrListMap LoadRefs, StoreRefs;
-    std::tie(LoadRefs, StoreRefs) = collectInstructions(BB);
-    Changed |= vectorizeChains(LoadRefs);
-    Changed |= vectorizeChains(StoreRefs);
+    if (BB->empty())
+      continue;
----------------
This would be malformed IR, each block has to at least have a terminator 


================
Comment at: llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:458
+    for (Instruction *I : ToErase) {
+      auto *GEP = dyn_cast<GetElementPtrInst>(getLoadStorePointerOperand(I));
+      if (I->use_empty())
----------------
jlebar wrote:
> tra wrote:
> > Is there a particular reason GEPs are special cased here? Dont we want to erase other unused arguments of the instructions we're deleting? Are we guaranteed that GEPs are the only argument kinds we're dealing with here? E.g. we may conceivably have an `inttoptr`.
> This is just matching the behavior of the original code.
> 
> I don't believe GEPs are special, but also I don't think we want to reeimplement a full DCE pass?
There is RecursivelyDeleteTriviallyDeadInstructions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149893