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

[weiwei via Phabricator via llvm-commits]

wwei added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4028
         return None;
-      auto *UserInst = cast_or_null<Instruction>(I->getUniqueUndroppableUser());
-      if (!UserInst)
+      if (!I->hasNUndroppableUsesOrMore(1))
         return None;
----------------
reames wrote:
> I don't think you need this check at all.  You can simple check that UniqueParent is non-null after the loop as that implies there must be at least one use.  
check removed


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4033
       BasicBlock *UserParent = nullptr;
+      BasicBlock *UniqueParent = nullptr;
 
----------------
nikic wrote:
> 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.
UniqueParent removed


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4062
       // not bother. SimplifyCFG should handle it.
       if (UserParent == BB || !DT.isReachableFromEntry(UserParent))
         return None;
----------------
nikic wrote:
> 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.
Thanks, these checks moved into for loop


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

https://reviews.llvm.org/D121585