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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

> This may either be an oversight, or an intentional improvement that guarantees that in this pass we don't repeat instcombine's mistake
> of relying on 3., but consistently add everything we want reprocessed into worklist.

I'd strongly suggest not to repeat InstCombine's mistakes :) Just the worklist iteration should be enough...



================
Comment at: llvm/include/llvm/Transforms/Utils/InstructionWorklist.h:35
 
   const char *DbgPrefix = "IC";
 
----------------
Drop this initializer, as initialized in ctor?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:952
     replaceValue(I, *NSI);
-    // Need erasing the store manually.
-    I.eraseFromParent();
+    eraseInstruction(I);
     return true;
----------------
Probably not necessary, as replaceValue() adds the old instruction to the worklist for DCE purposes?


================
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
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110171