[PATCH] D145223: [InstCombine] Combine binary operator of two phi node

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1314
+  // %add = phi i32 [%j, %bb0], [%i, %bb1]
+  // TODO: Support other binary operators
+  Constant *C = ConstantExpr::getBinOpIdentity(BO.getOpcode(), BO.getType(),
----------------
hiraditya wrote:
> nit: any remaining operators we are still missing?
Might make more sense to mention generalization to inst simplify.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1318
+  if (C) {
+    llvm::SmallVector<Value *, 4> NewIncomingValues;
+    if (all_of(llvm::zip(Phi0->operands(), Phi1->operands()),
----------------
Drop `llvm::`


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1320
+    if (all_of(llvm::zip(Phi0->operands(), Phi1->operands()),
+               [&](std::tuple<llvm::Use &, llvm::Use &> T) {
+                 auto &Phi0Use = std::get<0>(T);
----------------
This indent is ugly, I'd suggest moving this function into a variable.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1328
+                 Value *Phi1UseV = Phi1Use.get();
+                 if (dyn_cast<Constant>(Phi0UseV) == C)
+                   NewIncomingValues.push_back(Phi1UseV);
----------------
No need to dyn_cast, just comparison is enough.


================
Comment at: llvm/test/Transforms/InstCombine/phi.ll:1642
   ret i32 %add
 }
----------------
Add a test with `sub` to show that non-commutative operator is not folded?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145223