[PATCH] D65336: [InstSimplify] remove quadratic time looping (PR42771)

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/InstSimplifyPass.cpp:45-48
         // Don't waste time simplifying unused instructions.
-        if (!I->use_empty()) {
-          if (Value *V = SimplifyInstruction(I, SQ, ORE)) {
-            // Mark all uses for resimplification next time round the loop.
-            for (User *U : I->users())
-              Next->insert(cast<Instruction>(U));
-            I->replaceAllUsesWith(V);
-            ++NumSimplified;
-            Changed = true;
-          }
-        }
-        if (RecursivelyDeleteTriviallyDeadInstructions(I, SQ.TLI)) {
-          // RecursivelyDeleteTriviallyDeadInstruction can remove more than one
-          // instruction, so simply incrementing the iterator does not work.
-          // When instructions get deleted re-iterate instead.
-          BI = BB->begin();
-          BE = BB->end();
+        if (isInstructionTriviallyDead(&I)) {
+          DeadInstsInBB.push_back(&I);
+        } else if (Value *V = SimplifyInstruction(&I, SQ, ORE)) {
----------------
spatel wrote:
> foad wrote:
> > Why did you change the logic here? Now we might try to simplify an instruction that has no uses but is not trivially dead, which at least makes the comment on L45 slightly wrong.
> See comment at line 55 - just because an instruction (a call at least) has no uses does not mean it can not be simplified. I think we should use isInstructionTriviallyDead() as our point of truth here rather than mixing that with a use check.
> 
> I'll update the comment at line 45 if you agree.
> just because an instruction (a call at least) has no uses does not mean it can not be simplified

As I understand it, there is no point simplifying an instruction that has no uses. "Simplifying" does not change the instruction at all, it just returns a pointer to another (simpler) Value that users of the original instruction could use instead. If there are no users, there is no point doing this.


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

https://reviews.llvm.org/D65336