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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
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;
----------------
lebedev.ri wrote:
> Don't we also need `isSafeToSpeculativelyExecute()` check?
That's what the TODO comment and "mul_const_incoming0_speculatable" test are referring to (if I'm understanding the question).
Ie, for correctness, I think we only need to bail out for opcodes like div/rem. But for performance (and as a conservative first step), we probably don't want to hoist anything on this path if it isn't going to be executed unconditionally. Do we know that some other pass will sink the op later if possible?


================
Comment at: llvm/test/Transforms/InstCombine/zext-or-icmp.ll:42
 
 define i32 @dont_widen_undef() {
 ; CHECK-LABEL: @dont_widen_undef(
----------------
spatel wrote:
> aeubanks wrote:
> > these tests need to be changed to preserve testing of https://reviews.llvm.org/D30781
> I think these tests were already altered enough (there's no shift remaining in the existing CHECK lines) that they don't test the original problem (undefined shift). I'll try to come up with another way to cover that path.
Extra test added here:
417f5f4633e


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

https://reviews.llvm.org/D117110