[llvm] [InstCombine] Extend Phi-Icmp use to include or (PR #67682)

David Green via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 5 01:29:04 PDT 2023


================
@@ -1441,18 +1441,38 @@ Instruction *InstCombinerImpl::visitPHINode(PHINode &PN) {
         PHIUser->user_back() == &PN) {
       return replaceInstUsesWith(PN, PoisonValue::get(PN.getType()));
     }
-    // When a PHI is used only to be compared with zero, it is safe to replace
-    // an incoming value proved as known nonzero with any non-zero constant.
-    // For example, in the code below, the incoming value %v can be replaced
-    // with any non-zero constant based on the fact that the PHI is only used to
-    // be compared with zero and %v is a known non-zero value:
-    // %v = select %cond, 1, 2
-    // %p = phi [%v, BB] ...
-    //      icmp eq, %p, 0
-    auto *CmpInst = dyn_cast<ICmpInst>(PHIUser);
-    // FIXME: To be simple, handle only integer type for now.
-    if (CmpInst && isa<IntegerType>(PN.getType()) && CmpInst->isEquality() &&
-        match(CmpInst->getOperand(1), m_Zero())) {
+  }
+
+  // When a PHI is used only to be compared with zero, it is safe to replace
+  // an incoming value proved as known nonzero with any non-zero constant.
+  // For example, in the code below, the incoming value %v can be replaced
+  // with any non-zero constant based on the fact that the PHI is only used to
+  // be compared with zero and %v is a known non-zero value:
+  // %v = select %cond, 1, 2
+  // %p = phi [%v, BB] ...
+  //      icmp eq, %p, 0
+  // FIXME: To be simple, handle only integer type for now.
+  // Extend to 2 use of phi -> icmp and or(icmp). Putting a limit on number of
----------------
davemgreen wrote:

It might just be personal preference, but I don't love the "Extend to ..", just because when a developer is reading the code they don't really care how the algorithm was changed over time, they care about how it works now. (They can always read how things changed in the git history if needed).

I don't have much of an opinion whether this is limited to 2 uses or some N number of uses which is a small number, say 6 or 10. Either way it can use PN.hasNUsesOrMore(3).

Maybe for the comment just say something like
This handles a small number of uses to keep the complexity down, and an `icmp(or(phi))` can equally be replaced with any non-zero constant as the `or` will only add bits.

https://github.com/llvm/llvm-project/pull/67682


More information about the llvm-commits mailing list