[PATCH] D121585: [InstCombine] Sink instructions with multiple users in a successor block.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D121585#3380314 <https://reviews.llvm.org/D121585#3380314>, @reames wrote:

> Generally, I'm fine with the direction here.  My sole concern is potential compile time of scanning the entire user list.  (i.e. say we have 100 thousand uses in one block, and the very last one is in another block)   I could see us capping the number of scanned users (we do that a bunch of places) or would want to see some time time numbers showing we don't need to.

Agreed, this needs a cutoff.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4033
       BasicBlock *UserParent = nullptr;
+      BasicBlock *UniqueParent = nullptr;
 
----------------
The mixed usage of UserParent and UniqueParent here is odd. You'll want to move UserParent inside the loop and only use one variable outside it.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4062
       // not bother. SimplifyCFG should handle it.
       if (UserParent == BB || !DT.isReachableFromEntry(UserParent))
         return None;
----------------
It might make sense to move these checks to the first time we populate the user block, so we e.g. don't waste time visiting all users of an instruction in the same block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121585