[PATCH] D117110: [InstCombine] try to fold binop with phi operands

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1293-1297
+  auto *Phi0 = dyn_cast<PHINode>(BO.getOperand(0));
+  auto *Phi1 = dyn_cast<PHINode>(BO.getOperand(1));
+  if (!Phi0 || !Phi1 || !Phi0->hasOneUse() || !Phi1->hasOneUse() ||
+      Phi0->getNumOperands() != 2 || Phi1->getNumOperands() != 2)
+    return nullptr;
----------------
the fixme
```
  // TODO: This could miss matching a phi with 2 constant incoming values.
```
should be here, because uniform constant phi will be simplified into a plain constant


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1328-1333
+  // TODO: This check could be tightened to only apply to binops (div/rem) that
+  //       are not safe to speculatively execute. But that could allow hoisting
+  //       potentially expensive instructions (fdiv for example).
+  for (auto BBIter = BO.getParent()->begin(); &*BBIter != &BO; ++BBIter)
+    if (!isGuaranteedToTransferExecutionToSuccessor(&*BBIter))
+      return nullptr;
----------------
Don't we also need `isSafeToSpeculativelyExecute()` check?


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

https://reviews.llvm.org/D117110