[llvm] f2cfef3 - Be more mathematicly precise about definition of recurrence [NFC]

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2021-02-26T11:22:01-08:00
New Revision: f2cfef35966a24265b9e3c1b5ef8e64dcd5f431a

URL: https://github.com/llvm/llvm-project/commit/f2cfef35966a24265b9e3c1b5ef8e64dcd5f431a
DIFF: https://github.com/llvm/llvm-project/commit/f2cfef35966a24265b9e3c1b5ef8e64dcd5f431a.diff

LOG: Be more mathematicly precise about definition of recurrence [NFC]

This clarifies the interface of the matchSimpleRecurrence helper introduced in 8020be0b8 for non-commutative operators.  After ebd3aeba, I realized the original way I framed the routine was inconsistent.  For shifts, we only matched the the LHS form, but for sub we matched both and the caller wanted that information.  So, instead, we now consistently match both forms for non-commutative operators and the caller becomes responsible for filtering if needed.  I tried to put a clear warning in the header because I suspect the RHS form of e.g. a sub recurrence is non-obvious for most folks.  (It was for me.)

Added: 
    

Modified: 
    llvm/include/llvm/Analysis/ValueTracking.h
    llvm/lib/Analysis/ValueTracking.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 10b65a2b1383..584afc3c9205 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -748,11 +748,17 @@ constexpr unsigned MaxAnalysisRecursionDepth = 6;
   std::pair<Intrinsic::ID, bool>
   canConvertToMinOrMaxIntrinsic(ArrayRef<Value *> VL);
 
-  /// Attempt to match a simple recurrence cycle of the form:
-  ///   <Start, Op, Step> (using SCEV's notation)
-  /// In IR, this might look like:
+  /// Attempt to match a simple first order recurrence cycle of the form:
   ///   %iv = phi Ty [%Start, %Entry], [%Inc, %backedge]
   ///   %inc = binop %iv, %step
+  /// OR
+  ///   %iv = phi Ty [%Start, %Entry], [%Inc, %backedge]
+  ///   %inc = binop %step, %iv
+  ///
+  /// WARNING: For non-commutative operators, we will match both forms.  This
+  /// results in some odd recurrence structures.  Callers may wish to filter
+  /// out recurrences where the phi is not the LHS of the returned operator.
+  ///
   /// NOTE: This is intentional simple.  If you want the ability to analyze
   /// non-trivial loop conditons, see ScalarEvolution instead.
   bool matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,

diff  --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 07084c02e378..c62cfafd02b8 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1382,9 +1382,9 @@ static void computeKnownBitsFromOperator(const Operator *I,
       // If this is a shift recurrence, we know the bits being shifted in.
       // We can combine that with information about the start value of the
       // recurrence to conclude facts about the result.
-      if (Opcode == Instruction::LShr ||
-          Opcode == Instruction::AShr ||
-          Opcode == Instruction::Shl) {
+      if ((Opcode == Instruction::LShr || Opcode == Instruction::AShr ||
+           Opcode == Instruction::Shl) &&
+          BO->getOperand(0) == I) {
 
         // We have matched a recurrence of the form:
         // %iv = [R, %entry], [%iv.next, %backedge]
@@ -6051,21 +6051,10 @@ bool llvm::matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
     switch (Opcode) {
     default:
       continue;
+    // TODO: Expand list -- xor, div, gep, uaddo, etc..
     case Instruction::LShr:
     case Instruction::AShr:
-    case Instruction::Shl: {
-      Value *LL = LU->getOperand(0);
-      Value *LR = LU->getOperand(1);
-      // Find a recurrence.
-      if (LL == P)
-        L = LR;
-      else
-        continue; // Check for recurrence with L and R flipped.
-
-      break; // Match!
-    }
-
-    // TODO: Expand list -- xor, mul, div, gep, uaddo, etc..
+    case Instruction::Shl:
     case Instruction::Add:
     case Instruction::Sub:
     case Instruction::And:
@@ -6086,8 +6075,11 @@ bool llvm::matchSimpleRecurrence(const PHINode *P, BinaryOperator *&BO,
     };
 
     // We have matched a recurrence of the form:
-    // %iv = [R, %entry], [%iv.next, %backedge]
-    // %iv.next = binop %iv, L
+    //   %iv = [R, %entry], [%iv.next, %backedge]
+    //   %iv.next = binop %iv, L
+    // OR
+    //   %iv = [R, %entry], [%iv.next, %backedge]
+    //   %iv.next = binop L, %iv
     BO = cast<BinaryOperator>(LU);
     Start = R;
     Step = L;