[PATCH] D110171: [VectorCombine] Switch to using a worklist.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Transforms/Utils/InstructionWorklist.h:35
 
   const char *DbgPrefix = "IC";
 
----------------
nikic wrote:
> Drop this initializer, as initialized in ctor?
The `DbgPrefix` is gone now


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:108
+      Worklist.pushValue(OldI);
+    }
+  }
----------------
nikic wrote:
> Just `Worklist.pushValue(&Old)`. The Instruction check is exactly what `pushValue()` does relative to `push()`.
done thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:952
     replaceValue(I, *NSI);
-    // Need erasing the store manually.
-    I.eraseFromParent();
+    eraseInstruction(I);
     return true;
----------------
nikic wrote:
> Probably not necessary, as replaceValue() adds the old instruction to the worklist for DCE purposes?
`I` here is a store, so it won't be trivially dead


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:1079
+    // worklist, which means they'll end up popped from the worklist in-order.
+    while (Instruction *I = Worklist.popDeferred()) {
+      // Check to see if we can DCE the instruction. We do this already here to
----------------
nikic wrote:
> Do we actually need the deferred worklist for VectorCombine (does it make a difference for any test cases)? InstCombine has some pretty complicated worklist management, it might be preferable to forgo it if we don't need it. (No strong opinion though.)
Not really, I removed the deferred handling .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110171