[PATCH] D75008: [InstCombine] DCE instructions earlier

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineInternal.h:727
         if (auto *Inst = dyn_cast<Instruction>(Operand))
-          Worklist.push(Inst);
+          Worklist.add(Inst);
     }
----------------
spatel wrote:
> I guess it's independent of this patch, but I'm confused about when it's appropriate to push() vs. add(). Will we eventually reach a state where push() is private to the worklist implementation, and all the user code should use add()?
Generally yes, user code should use add() and things are slowly moving in that direction...

Normally the choice of add() (FIFO, what you usually want) vs push() (LIFO) is about order. In this case there is no clear order in which the operands should be processed, so either is fine. I'm only using add() because the extra DCE code added in this patch works by processing the deferred worklist.

I think in the future, we may want to have a separate worklist only for DCE (instead of reusing the deferred worklist), and separate out add() vs addForDCE(). I'm leaving that for later, as I'm not sure whether it's sufficient to only perform DCE (rather than a full revisit) if the use-count drops.


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

https://reviews.llvm.org/D75008