[PATCH] D13063: Faster SimplifyInstructionsInBlock

[escha via llvm-commits]

escha 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
----------------
reames wrote:
> You need to pass in TLI here or you have a regression over the previous code when handling target library routines.
Ah, right, I missed that. Will fix.

================
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))
----------------
reames wrote:
> Use a foreach operands loop here.
> 
> Actually, the style used in RecursivelyDeleteTriviallyDeadInstructions is probably preferable here.  It doesn't need the extra Operands array.
Ah, I see, that should work.

================
Comment at: lib/Transforms/Utils/Local.cpp:461
@@ +460,3 @@
+    // parent block.
+    if (I->getParent())
+      I->eraseFromParent();
----------------
reames wrote:
> 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.
Okay, I was just matching the original code; I didn't quite understand what it's there for?

================
Comment at: lib/Transforms/Utils/Local.cpp:497
@@ +496,3 @@
+    // worklist from an earlier visit.
+    WorkList.remove(I);
+    MadeChange |= simplifyAndDCEInstruction(I, WorkList, DL);
----------------
reames wrote:
> 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?
Removal from a SmallSetVector should be fast, shouldn't it?

Also, one of the main problems I encountered earlier is specifically that WeakVHs are *incredibly slow* and constructing them took a large portion of this function's time.


Repository:
  rL LLVM

http://reviews.llvm.org/D13063