[PATCH] D109700: [InstCombine] Improve TryToSinkInstruction with multiple uses

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The change looks reasonable to me.



================
Comment at: llvm/lib/IR/Value.cpp:169
+  User *Result = nullptr;
   for (Use &U : uses()) {
     if (!U.getUser()->isDroppable()) {
----------------
Might as well iterate over `users()` and avoid the three `U.getUser()` calls?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3821
         return None;
-      Use *SingleUse = I->getSingleUndroppableUse();
-      if (!SingleUse)
+      auto *UserInst = dyn_cast_or_null<Instruction>(I->getUniqueUndroppableUser());
+      if (!UserInst)
----------------
Dropping `dyn_` should be fine here -- user of instruction is always an instruction.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3830
+      if (PHINode *PN = dyn_cast<PHINode>(UserInst)) {
+        for (unsigned i =0; i < PN->getNumIncomingValues(); i++)
+          if (PN->getIncomingValue(i) == I) {
----------------
nit: Missing space after `=`. Also, please add braces for the `for`, as the inner code has braces.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:165
+        auto *UniqueUser =
+            dyn_cast_or_null<Instruction>(RK.WasOn->getUniqueUndroppableUser());
+        if (UniqueUser == InstBeingModified)
----------------
Is the cast here needed?


================
Comment at: llvm/test/Transforms/InstCombine/sink_instruction.ll:59
 define i32 @test3(i32* nocapture readonly %P, i32 %i) {
+; CHECK-LABEL: test3(
 entry:
----------------
Please precommit tests, including generation of all check lines in this file using update_test_checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109700