[PATCH] D102511: Do actual DCE in LoopUnroll

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

@nikic Didn't see your comments until after pushing, will address post-commit shortly.



================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:226
   const DataLayout &DL = L->getHeader()->getModule()->getDataLayout();
+  SmallVector<WeakTrackingVH, 16> DeadInsts;
   for (BasicBlock *BB : L->getBlocks()) {
----------------
nikic wrote:
> nikic wrote:
> > Why do we need WeakTrackingVH here?
> Err, ignore this comment, I submitted it by accident -- it's incompatible with the other one, which requires WeakTrackingVH in the API.
I was concerned about visit order issues.  Taking another look, I think this is redundant.  


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:239
 
-  // TODO: after peeling or unrolling, previously loop variant conditions are
-  // likely to fold to constants, eagerly propagating those here will require
-  // fewer cleanup passes to be run.  Alternatively, a LoopEarlyCSE might be
-  // appropriate.
+  while (!DeadInsts.empty()) {
+    Value *V = DeadInsts.pop_back_val();
----------------
nikic wrote:
> Why not use the `RecursivelyDeleteTriviallyDeadInstructions(DeadInsts)` overload here?
Good question, will fix this and other site in same file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102511