[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:1329-1334
+  Builder.SetInsertPoint(PredBlockBranch);
+  Value *NewBO = Builder.CreateBinOp(BO.getOpcode(),
+                                     Phi0->getIncomingValueForBlock(OtherBB),
+                                     Phi1->getIncomingValueForBlock(OtherBB));
+  if (auto *NotFoldedNewBO = dyn_cast<BinaryOperator>(NewBO))
+    NotFoldedNewBO->copyIRFlags(&BO);
----------------
luna wrote:
> I'm wondering if it's idiomatic to validate that this instruction could also be simplified away in later iterations of instruction-combine, and only proceed with the fold if yes.
> 
> e.g., in https://godbolt.org/z/fKfondqrn, `%res = or i64 %retval.sroa.3.0.extract.shift, %phi.cast` instruction is inserted into `if.then` block, and could be simplified away.
In general, instcombine tries to do the smallest transform that still results in an improvement. It can then chain together a series of minimal transforms to achieve the larger goal. 

So if we can make this patch safe and still profitable while creating a new binop, then that's better than limiting a larger transform to the case where we eliminate the binop completely.


================
Comment at: llvm/test/Transforms/InstCombine/zext-or-icmp.ll:42
 
 define i32 @dont_widen_undef() {
 ; CHECK-LABEL: @dont_widen_undef(
----------------
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.


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

https://reviews.llvm.org/D117110