[PATCH] D115914: [InstCombine] Fold two PHI operands of `or` conditionally.

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

this seems reasonable, I like this approach
needs more test coverage though



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1292
 
+Value *InstCombinerImpl::couldSimplifyInBasicBlock(BinaryOperator &I, Value *V0,
+                                                   Value *V1, BasicBlock *BB) {
----------------
I think we should directly call `SimplifyInstruction()`, no need to constrict this to `or`
and the naming on this function isn't great anyway, it returns a `Value*` but starts with `could`


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1310
+  // blocks.
+  if ((NumPHIValues != 2) || (NumPHIValues != PN1->getNumIncomingValues())) {
+    return nullptr;
----------------
need a test for this




================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1321
+
+  if ((PN0_BB0 == PN1_BB1) && (PN0_BB1 == PN1_BB0)) {
+    std::swap(PN0_BB0, PN0_BB1);
----------------
need a test for this


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1335
+  // Validates both PHI nodes have one use, for simplicity.
+  if (!(PN0->hasOneUse() && PN1->hasOneUse())) {
+    return nullptr;
----------------
need a test for this


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1339
+
+  if (couldSimplifyInBasicBlock(I, PN0_VAL0, PN1_VAL0, PN0_BB0) &&
+      couldSimplifyInBasicBlock(I, PN0_VAL1, PN1_VAL1, PN1_BB1)) {
----------------
early return is nicer


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1339
+
+  if (couldSimplifyInBasicBlock(I, PN0_VAL0, PN1_VAL0, PN0_BB0) &&
+      couldSimplifyInBasicBlock(I, PN0_VAL1, PN1_VAL1, PN1_BB1)) {
----------------
aeubanks wrote:
> early return is nicer
I think even if we only simplify one of the two values it's worth it, don't need to constrict to this to requiring both values to be simplified. can be a TODO though


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1366
+    Builder.SetInsertPoint(PN0_BB0->getTerminator());
+    Value *ValFromBB0 = Builder.CreateBinOp(I.getOpcode(), PN0_VAL0, PN1_VAL0);
+    NewPN->addIncoming(ValFromBB0, PN0_BB0);
----------------
can we use the value above from the call to `couldSimplifyInBasicBlock()`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:4478
   unwrap(PM)->add(createInstructionCombiningPass());
-}
+}
----------------
unintentional change? perhaps some editor config needs to be changed?


================
Comment at: llvm/test/Transforms/InstCombine/fold-phi-of-binary-op.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -instcombine -S < %s | FileCheck %s
----------------
regarding the file name, it's more of a "binary-op-of-phi" than "phi-of-binary-op"


================
Comment at: llvm/test/Transforms/InstCombine/fold-phi-of-binary-op.ll:9
+; and IR get simplified.
+define dso_local i64 @_Z5ParsebPF6RetValvE(i1 zeroext %test, i64 ()* nocapture %p) local_unnamed_addr {
+; CHECK-LABEL: @_Z5ParsebPF6RetValvE(
----------------
it'd be nice to simplify the IR a bit, e.g. simplify the name, remove `dso_local`, `local_unnamed_addr`, `nocapture`, etc


================
Comment at: llvm/test/Transforms/InstCombine/zext-or-icmp.ll:49
 ; CHECK:       block2:
-; CHECK-NEXT:    [[CMP_I:%.*]] = phi i1 [ false, [[BLOCK1:%.*]] ], [ true, [[ENTRY:%.*]] ]
-; CHECK-NEXT:    [[CMP115:%.*]] = phi i1 [ true, [[BLOCK1]] ], [ false, [[ENTRY]] ]
-; CHECK-NEXT:    [[CMP1:%.*]] = or i1 [[CMP_I]], [[CMP115]]
-; CHECK-NEXT:    [[CONV2:%.*]] = zext i1 [[CMP1]] to i32
-; CHECK-NEXT:    ret i32 [[CONV2]]
+; CHECK-NEXT:    ret i32 1
 ;
----------------
this test no longer tests https://reviews.llvm.org/D30781, we need to modify this test somehow
maybe something like

`select i1 %param, i32 33, 0` instead of `%m.011 = phi ...`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115914