[ValueTracking] Add `matchSimpleBinaryIntrinsicRecurrence` helper #145964

antoniofrighetto · 2025-06-26T20:43:52Z

Similarly to what it is being done to match simple recurrence cycle relations, attempt to match value-accumulating recurrences of kind:

  %umax.acc = phi i8 [ %umax, %backedge ], [ %a, %entry ]
  %umax = call i8 @llvm.umax.i8(i8 %umax.acc, i8 %b)

Preliminary work to let InstCombine avoid folding such recurrences, so that simple loop-invariant computation may get hoisted. Minor opportunity to refactor out code as well.

llvmbot · 2025-06-26T20:44:25Z

@llvm/pr-subscribers-llvm-analysis

Author: Antonio Frighetto (antoniofrighetto)

Changes

Similarly to what it is being done to match simple recurrence cycle relations, attempt to match value-accumulating recurrences of kind:

  %umax.acc = phi i8 [ %umax, %backedge ], [ %a, %entry ]
  %umax = call i8 @<!-- -->llvm.umax.i8(i8 %umax.acc, i8 %b)

Preliminary work to let InstCombine avoid folding such recurrences, so that simple loop-invariant computation may get hoisted. Minor opportunity to refactor out code as well.

Full diff: https://github.com/llvm/llvm-project/pull/145964.diff

2 Files Affected:

(modified) llvm/include/llvm/Analysis/ValueTracking.h (+16)
(modified) llvm/lib/Analysis/ValueTracking.cpp (+49-30)

diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index c804f551f5a75..384e9915a3bca 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -21,6 +21,7 @@
 #include "llvm/IR/FMF.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/Support/Compiler.h"
 #include <cassert>
@@ -965,6 +966,21 @@ LLVM_ABI bool matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
 LLVM_ABI bool matchSimpleRecurrence(const BinaryOperator *I, PHINode *&P,
                                     Value *&Start, Value *&Step);
 
+/// Attempt to match a simple value-accumulating recurrence of the form:
+///   %llvm.intrinsic.acc = phi Ty [%Init, %Entry], [%llvm.intrinsic, %backedge]
+///   %llvm.intrinsic = call Ty @llvm.intrinsic(%Invariant, %llvm.intrinsic.acc)
+/// OR
+///   %llvm.intrinsic.acc = phi Ty [%Init, %Entry], [%llvm.intrinsic, %backedge]
+///   %llvm.intrinsic = call Ty @llvm.intrinsic(%llvm.intrinsic.acc, %Invariant)
+///
+/// The recurrence relation is of kind:
+///   X_0 = %a (initial value),
+///   X_i = call @llvm.binary.intrinsic(X_i-1, %b)
+/// Where both %a and %b may be loop-invariant.
+LLVM_ABI bool matchSimpleBinaryIntrinsicRecurrence(const IntrinsicInst *I,
+                                                   PHINode *&P, Value *&Init,
+                                                   Value *&Invariant);
+
 /// Return true if RHS is known to be implied true by LHS.  Return false if
 /// RHS is known to be implied false by LHS.  Otherwise, return std::nullopt if
 /// no implication can be made. A & B must be i1 (boolean) values or a vector of
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 93c22212a27ce..fe831236aef54 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -9070,46 +9070,48 @@ llvm::canConvertToMinOrMaxIntrinsic(ArrayRef<Value *> VL) {
   return {Intrinsic::not_intrinsic, false};
 }
 
-bool llvm::matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
-                                 Value *&Start, Value *&Step) {
+template <typename InstTy>
+static bool
+matchTwoInputRecurrence(const PHINode *PN, InstTy *&Inst, Value *&Init,
+                        Value *&OtherOp,
+                        std::function<bool(const InstTy *)> Pred = nullptr) {
   // Handle the case of a simple two-predecessor recurrence PHI.
   // There's a lot more that could theoretically be done here, but
   // this is sufficient to catch some interesting cases.
   // TODO: Expand list -- gep, uadd.sat etc.
-  if (P->getNumIncomingValues() != 2)
+  if (PN->getNumIncomingValues() != 2)
     return false;
 
-  for (unsigned i = 0; i != 2; ++i) {
-    Value *L = P->getIncomingValue(i);
-    Value *R = P->getIncomingValue(!i);
-    auto *LU = dyn_cast<BinaryOperator>(L);
-    if (!LU)
-      continue;
-    Value *LL = LU->getOperand(0);
-    Value *LR = LU->getOperand(1);
-
-    // Find a recurrence.
-    if (LL == P)
-      L = LR;
-    else if (LR == P)
-      L = LL;
-    else
-      continue; // Check for recurrence with L and R flipped.
-
-    // We have matched a recurrence of the form:
-    //   %iv = [R, %entry], [%iv.next, %backedge]
-    //   %iv.next = binop %iv, L
-    // OR
-    //   %iv = [R, %entry], [%iv.next, %backedge]
-    //   %iv.next = binop L, %iv
-    BO = LU;
-    Start = R;
-    Step = L;
-    return true;
+  for (unsigned I = 0; I != 2; ++I) {
+    if (auto *Operation = dyn_cast<InstTy>(PN->getIncomingValue(I))) {
+      if (Pred && !(Pred)(Operation))
+        continue;
+
+      Value *LHS = Operation->getOperand(0);
+      Value *RHS = Operation->getOperand(1);
+      if (LHS != PN && RHS != PN)
+        continue;
+
+      Inst = Operation;
+      Init = PN->getIncomingValue(!I);
+      OtherOp = (LHS == PN) ? RHS : LHS;
+      return true;
+    }
   }
   return false;
 }
 
+bool llvm::matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
+                                 Value *&Start, Value *&Step) {
+  // We have matched a recurrence of the form:
+  //   %iv = [Start, %entry], [%iv.next, %backedge]
+  //   %iv.next = binop %iv, Step
+  // Or:
+  //   %iv = [Start, %entry], [%iv.next, %backedge]
+  //   %iv.next = binop Step, %iv
+  return matchTwoInputRecurrence(P, BO, Start, Step);
+}
+
 bool llvm::matchSimpleRecurrence(const BinaryOperator *I, PHINode *&P,
                                  Value *&Start, Value *&Step) {
   BinaryOperator *BO = nullptr;
@@ -9119,6 +9121,23 @@ bool llvm::matchSimpleRecurrence(const BinaryOperator *I, PHINode *&P,
   return P && matchSimpleRecurrence(P, BO, Start, Step) && BO == I;
 }
 
+bool llvm::matchSimpleBinaryIntrinsicRecurrence(const IntrinsicInst *I,
+                                                PHINode *&P, Value *&Init,
+                                                Value *&Invariant) {
+  IntrinsicInst *II = nullptr;
+  P = dyn_cast<PHINode>(I->getArgOperand(0));
+  if (!P)
+    P = dyn_cast<PHINode>(I->getArgOperand(1));
+
+  std::function<bool(const IntrinsicInst *)> IsBinaryOrMinMaxFn =
+      [](const IntrinsicInst *I) {
+        return isa<BinaryOpIntrinsic, MinMaxIntrinsic>(I);
+      };
+  return P &&
+         matchTwoInputRecurrence(P, II, Init, Invariant, IsBinaryOrMinMaxFn) &&
+         II == I;
+}
+
 /// Return true if "icmp Pred LHS RHS" is always true.
 static bool isTruePredicate(CmpInst::Predicate Pred, const Value *LHS,
                             const Value *RHS) {

dtcxzyw

Can you add some unit tests to llvm/unittests/Analysis/ValueTrackingTest.cpp?

dtcxzyw · 2025-06-27T06:01:00Z

llvm/lib/Analysis/ValueTracking.cpp

+  if (!P)
+    P = dyn_cast<PHINode>(I->getArgOperand(1));
+
+  std::function<bool(const IntrinsicInst *)> IsBinaryOrMinMaxFn =


Can we run the predicate after matching the recurrence?

We could run the predicate after matching the recurrence, but that would imply computing all the recurrence needlessly, if the intrinsic turned out not to be a candidate one (as we're passing a general IntrinsicInst), in addition to checking intrinsic operands size (as not to go out of bound when retrieving the Operation ops). Switched this to using a lightweight llvm::function_ref instead, I think it should be quite better.

dtcxzyw · 2025-06-27T06:03:09Z

llvm/lib/Analysis/ValueTracking.cpp

+
+  std::function<bool(const IntrinsicInst *)> IsBinaryOrMinMaxFn =
+      [](const IntrinsicInst *I) {
+        return isa<BinaryOpIntrinsic, MinMaxIntrinsic>(I);


xx.with.overflow should not be accepted. BTW, I'd like to handle fmax/fmin as well.

Suggested change

return isa<BinaryOpIntrinsic, MinMaxIntrinsic>(I);

return isa<SaturatingInst, MinMaxIntrinsic>(I);

antoniofrighetto · 2025-06-27T10:32:43Z

Can you add some unit tests to llvm/unittests/Analysis/ValueTrackingTest.cpp?

Sure thing (my bad not adding them earlier).

nikic · 2025-06-27T10:35:42Z

llvm/lib/Analysis/ValueTracking.cpp

+                      IID == Intrinsic::minimum || IID == Intrinsic::maximum ||
+                      IID == Intrinsic::minimumnum ||
+                      IID == Intrinsic::maximumnum;
+             }) &&


I would expect that the caller is responsible for checking the intrinsic ID. We should only check that it's a two argument intrinsic (maybe with same arg and ret types) here.

Not sure got this, the caller here for checking the intrinsic ID, the one to be set, should be matchSimpleBinaryIntrinsicRecurrence itself. The lambda is used to check whether the intrinsic found in matchTwoInputRecurrence is the expected one. We can check that I is a two argument intrinsic, but we would still need to check, once matchTwoInputRecurrence returns, that the new intrinsic is the expected one.

I don't understand why this function needs to check the intrinsic ID at all. We recently removed the check for opcodes form the BinaryOperator variant, leaving the choice of which intrinsics to support to the caller.

Oh, I stand corrected, it suffices to check II == I once matchTwoInputRecurrence returns.

nikic

LGTM

nikic · 2025-06-27T15:09:08Z

llvm/include/llvm/Analysis/ValueTracking.h

@@ -965,6 +966,21 @@ LLVM_ABI bool matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
 LLVM_ABI bool matchSimpleRecurrence(const BinaryOperator *I, PHINode *&P,
                                    Value *&Start, Value *&Step);

+/// Attempt to match a simple value-accumulating recurrence of the form:
+///   %llvm.intrinsic.acc = phi Ty [%Init, %Entry], [%llvm.intrinsic, %backedge]
+///   %llvm.intrinsic = call Ty @llvm.intrinsic(%Invariant, %llvm.intrinsic.acc)


Don't call this Invariant, here and elsewhere. This matcher does not actually guarantee it's invariant (just like the one above doesn't guarantee Step is invariant).

The pattern you want to match has an invariant there, but a typical reduction pattern wouldn#t.

Right, was mentioned in the doc-comment, but the variable name didn't reflect this; updated, thanks!

nikic · 2025-06-27T15:27:39Z

llvm/include/llvm/Analysis/ValueTracking.h

+/// The recurrence relation is of kind:
+///   X_0 = %a (initial value),
+///   X_i = call @llvm.binary.intrinsic(X_i-1, %b)
+/// Where both %a and %b may be loop-invariant.


I think this last sentence is more confusing than helpful. (%a is always loop invariant, %b can be either.)

nikic · 2025-06-27T15:36:03Z

llvm/include/llvm/Analysis/ValueTracking.h

+/// The recurrence relation is of kind:
+///   X_0 = %a (initial value),
+///   X_i = call @llvm.binary.intrinsic(X_i-1, %b)
+/// Where %b may be a loop-invariant value.


I still find the phrasing here confusing...

Suggested change

/// Where %b may be a loop-invariant value.

/// Where %b is not required to be loop-invariant.

Thank you, ameliorated this.

Similarly to what it is being done to match simple recurrence cycle relations, attempt to match value-accumulating recurrences of kind: ``` %umax.acc = phi i8 [ %umax, %backedge ], [ %a, %entry ] %umax = call i8 @llvm.umax.i8(i8 %umax.acc, i8 %b) ``` Preliminary work to let InstCombine avoid folding such recurrences, so that simple loop-invariant computation may get hoisted. Minor opportunity to refactor out code as well.

nikic · 2025-06-27T19:30:32Z

llvm/include/llvm/Analysis/ValueTracking.h

@@ -21,6 +21,7 @@
 #include "llvm/IR/FMF.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"


This should be a forward declaration. The include causes a build time regression.

Fixed in 69b8e59.

antoniofrighetto requested a review from dtcxzyw June 26, 2025 20:43

antoniofrighetto requested a review from nikic as a code owner June 26, 2025 20:43

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jun 26, 2025

dtcxzyw reviewed Jun 27, 2025

View reviewed changes

nikic reviewed Jun 27, 2025

View reviewed changes

nikic approved these changes Jun 27, 2025

View reviewed changes

nikic reviewed Jun 27, 2025

View reviewed changes

nikic approved these changes Jun 27, 2025

View reviewed changes

antoniofrighetto force-pushed the feature/vt-match-recurr-binop-intrns branch from 413db68 to c11ea44 Compare June 27, 2025 17:04

antoniofrighetto merged commit c11ea44 into llvm:main Jun 27, 2025
5 of 7 checks passed

nikic reviewed Jun 27, 2025

View reviewed changes

	return isa<BinaryOpIntrinsic, MinMaxIntrinsic>(I);
	return isa<SaturatingInst, MinMaxIntrinsic>(I);

	/// Where %b may be a loop-invariant value.
	/// Where %b is not required to be loop-invariant.

[ValueTracking] Add matchSimpleBinaryIntrinsicRecurrence helper #145964

[ValueTracking] Add matchSimpleBinaryIntrinsicRecurrence helper #145964

Uh oh!

Conversation

antoniofrighetto commented Jun 26, 2025

Uh oh!

llvmbot commented Jun 26, 2025

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniofrighetto commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniofrighetto Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[ValueTracking] Add `matchSimpleBinaryIntrinsicRecurrence` helper #145964

[ValueTracking] Add `matchSimpleBinaryIntrinsicRecurrence` helper #145964

antoniofrighetto Jun 27, 2025 •

edited

Loading