[PATCH] D13063: Faster SimplifyInstructionsInBlock

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Utils/Local.cpp:427
@@ +426,3 @@
+                          const DataLayout &DL) {
+  if (isInstructionTriviallyDead(I, nullptr)) {
+    // Loop over all of the values that the instruction uses. If there are
----------------
You need to pass in TLI here or you have a regression over the previous code when handling target library routines.

================
Comment at: lib/Transforms/Utils/Local.cpp:432
@@ +431,3 @@
+    SmallVector<Instruction *, 8> Operands;
+    for (User::op_iterator OI = I->op_begin(), E = I->op_end(); OI != E; ++OI)
+      if (Instruction *Used = dyn_cast<Instruction>(*OI))
----------------
Use a foreach operands loop here.

Actually, the style used in RecursivelyDeleteTriviallyDeadInstructions is probably preferable here.  It doesn't need the extra Operands array.

================
Comment at: lib/Transforms/Utils/Local.cpp:461
@@ +460,3 @@
+    // parent block.
+    if (I->getParent())
+      I->eraseFromParent();
----------------
I think this should just be an assert at the top of the function.  Or at least, if you're going to handle this in one case, handle it in the dead instruction case as well.

================
Comment at: lib/Transforms/Utils/Local.cpp:497
@@ +496,3 @@
+    // worklist from an earlier visit.
+    WorkList.remove(I);
+    MadeChange |= simplifyAndDCEInstruction(I, WorkList, DL);
----------------
This remove seems problematic.  Isn't this potentially O(n^2) in the instructions in the BB?  Or is the argument that we're walking through the BB in an order where they shouldn't generally be in the Worklist?

Would it be cleaner/safer to use a SmallSetVector of Weak Value Handles and then just skip the deleted nodes?

Or alternatively, just not visit the instruction here if it's already in the worklist?


Repository:
  rL LLVM

http://reviews.llvm.org/D13063