[PATCH] D11648: InstCombinePHI: Partial simplification of identity operations

[Jakub Kuderski via llvm-commits]

kuhar added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:846-855
@@ +845,12 @@
+
+  // Cannot hoist an operation if the incoming BasicBlock has conditional branch
+  // and when the new instruction could result in trap.
+  // If the Terminator is not a BranchInst we don't apply the transformation,
+  // since it's not obvious if that'd be a performance win and if it'd be safe.
+  if (const auto *BI = dyn_cast<BranchInst>(Terminator)) {
+    if (BI->isConditional() && (PNUser.getOpcode() == Instruction::UDiv ||
+                                PNUser.getOpcode() == Instruction::SDiv))
+      return nullptr;
+  } else
+    return nullptr;
+
----------------
kuhar wrote:
> majnemer wrote:
> > Why not just check if `BB->getUniqueSuccessor() != nullptr`?
> Do you also suggest removing this check for UDiv/SDiv with conditional branch?
I can either continue checking if an instruction is a branch and make sure that if it's conditional branch hoisting operation won't potentially cause a trap (`sdiv`/`udiv`); or I can only apply the optimization to unconditional branches.

Currently I follow the first option, so I cast the instruction to `BranchInst` and then check if it's conditional. `getUniqueSuccessor` won't simplify anything here (I think).


Repository:
  rL LLVM

http://reviews.llvm.org/D11648