[IVDesc] Unify RecurKinds [I|F]AnyOf #118393

artagnon · 2024-12-02T20:10:49Z

No description provided.

llvmbot · 2024-12-02T20:11:35Z

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-llvm-analysis

Author: Ramkumar Ramachandra (artagnon)

Changes

Checking RecurKind::FAnyOf in isRecurrenceDescriptor() is wasted work when it already checks RecurKind::IAnyOf. Affect a minor adjustment to the code to facilitate skipping the RecurKind::FAnyOf check, and strip the check. The patch has the side-effect of rewriting some flaky code, which would match an ICmp having the reduction phi as an operand with IAnyOf, and due to NumCmpSelectPatternInst != 1 (the select redux is also matched), it would have to rely on failing to match an FCmp with FAnyOf, setting NumCmpSelectPatternInst = 1, and successfully vectorizing an IAnyOf pattern with the incorrect debug output. There is a test for this already in select-cmp.ll: select_i32_from_icmp_same_inputs.

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

1 Files Affected:

(modified) llvm/lib/Analysis/IVDescriptors.cpp (+6-15)

diff --git a/llvm/lib/Analysis/IVDescriptors.cpp b/llvm/lib/Analysis/IVDescriptors.cpp
index 23e11bdbeab4c5..9678fcdcbd7c0e 100644
--- a/llvm/lib/Analysis/IVDescriptors.cpp
+++ b/llvm/lib/Analysis/IVDescriptors.cpp
@@ -415,12 +415,9 @@ bool RecurrenceDescriptor::AddReductionVar(
     if (IsAPhi && Cur != Phi && !areAllUsesIn(Cur, VisitedInsts))
       return false;
 
-    if ((isIntMinMaxRecurrenceKind(Kind) || Kind == RecurKind::IAnyOf) &&
-        (isa<ICmpInst>(Cur) || isa<SelectInst>(Cur)))
-      ++NumCmpSelectPatternInst;
-    if ((isFPMinMaxRecurrenceKind(Kind) || Kind == RecurKind::FAnyOf) &&
-        (isa<FCmpInst>(Cur) || isa<SelectInst>(Cur)))
-      ++NumCmpSelectPatternInst;
+    if (isIntMinMaxRecurrenceKind(Kind) || isAnyOfRecurrenceKind(Kind))
+      if (match(Cur, m_Select(m_Cmp(), m_Value(), m_Value())))
+        ++NumCmpSelectPatternInst;
 
     // Check  whether we found a reduction operator.
     FoundReduxOp |= !IsAPhi && Cur != Start;
@@ -500,7 +497,7 @@ bool RecurrenceDescriptor::AddReductionVar(
   // This means we have seen one but not the other instruction of the
   // pattern or more than just a select and cmp. Zero implies that we saw a
   // llvm.min/max intrinsic, which is always OK.
-  if (isMinMaxRecurrenceKind(Kind) && NumCmpSelectPatternInst != 2 &&
+  if (isMinMaxRecurrenceKind(Kind) && NumCmpSelectPatternInst != 1 &&
       NumCmpSelectPatternInst != 0)
     return false;
 
@@ -889,8 +886,8 @@ bool RecurrenceDescriptor::isReductionPHI(PHINode *Phi, Loop *TheLoop,
   }
   if (AddReductionVar(Phi, RecurKind::IAnyOf, TheLoop, FMF, RedDes, DB, AC, DT,
                       SE)) {
-    LLVM_DEBUG(dbgs() << "Found an integer conditional select reduction PHI."
-                      << *Phi << "\n");
+    LLVM_DEBUG(dbgs() << "Found an conditional select reduction PHI." << *Phi
+                      << "\n");
     return true;
   }
   if (AddReductionVar(Phi, RecurKind::FMul, TheLoop, FMF, RedDes, DB, AC, DT,
@@ -913,12 +910,6 @@ bool RecurrenceDescriptor::isReductionPHI(PHINode *Phi, Loop *TheLoop,
     LLVM_DEBUG(dbgs() << "Found a float MIN reduction PHI." << *Phi << "\n");
     return true;
   }
-  if (AddReductionVar(Phi, RecurKind::FAnyOf, TheLoop, FMF, RedDes, DB, AC, DT,
-                      SE)) {
-    LLVM_DEBUG(dbgs() << "Found a float conditional select reduction PHI."
-                      << " PHI." << *Phi << "\n");
-    return true;
-  }
   if (AddReductionVar(Phi, RecurKind::FMulAdd, TheLoop, FMF, RedDes, DB, AC, DT,
                       SE)) {
     LLVM_DEBUG(dbgs() << "Found an FMulAdd reduction PHI." << *Phi << "\n");

fhahn

This effectively makes FAnyOf dead, right? In that case it should be removed and IAnyOf -> AnyOf?

artagnon · 2024-12-03T09:44:32Z

This effectively makes FAnyOf dead, right? In that case it should be removed and IAnyOf -> AnyOf?

There is still FAnyOf matching with FCmp, and IAnyOf matching with ICmp, so I don't think it can be removed. See getReductionOpChain's call to getOpcode for example.

See also the return statement in isAnyOfPattern:

  return InstDesc(I, isa<ICmpInst>(I->getOperand(0)) ? RecurKind::IAnyOf
                                                     : RecurKind::FAnyOf);

david-arm · 2024-12-03T10:19:16Z

This effectively makes FAnyOf dead, right? In that case it should be removed and IAnyOf -> AnyOf?

There is still FAnyOf matching with FCmp, and IAnyOf matching with ICmp, so I don't think it can be removed. See getReductionOpChain's call to getOpcode for example.

See also the return statement in isAnyOfPattern:
  return InstDesc(I, isa<ICmpInst>(I->getOperand(0)) ? RecurKind::IAnyOf
                                                     : RecurKind::FAnyOf);

I agree with @fhahn. Looking at the code after this patch lands there is no logical reason for distinguishing between IAnyOf and FAnyOf. You should be able to condense the two RecurKind types into a single AnyOf I think. When creating InstDesc you just always initialise it with RecurKind::AnyOf and in isAnyOfRecurrenceKind just check for AnyOf. It looks like the only place we actually distinguish between the two is in RecurrenceDescriptor::getOpcode, but you can probably infer the opcode from the type?

david-arm · 2024-12-03T10:20:52Z

Also, I think if this patch is fixing a bug (which you implied from the commit message) then it probably shouldn't have NFC in the title or commit message? That sounds like a functional change.

artagnon · 2024-12-03T11:39:04Z

This effectively makes FAnyOf dead, right? In that case it should be removed and IAnyOf -> AnyOf?

There is still FAnyOf matching with FCmp, and IAnyOf matching with ICmp, so I don't think it can be removed. See getReductionOpChain's call to getOpcode for example.
See also the return statement in isAnyOfPattern:
  return InstDesc(I, isa<ICmpInst>(I->getOperand(0)) ? RecurKind::IAnyOf
                                                     : RecurKind::FAnyOf);
I agree with @fhahn. Looking at the code after this patch lands there is no logical reason for distinguishing between IAnyOf and FAnyOf. You should be able to condense the two RecurKind types into a single AnyOf I think. When creating InstDesc you just always initialise it with RecurKind::AnyOf and in isAnyOfRecurrenceKind just check for AnyOf. It looks like the only place we actually distinguish between the two is in RecurrenceDescriptor::getOpcode, but you can probably infer the opcode from the type?

Thanks, both. FAnyOf has now been stripped altogether, and IAnyOf has been renamed to AnyOf.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

llvm/lib/Analysis/IVDescriptors.cpp

artagnon · 2024-12-03T15:49:28Z

Rebased.

Mel-Chen

Thanks your contribution. I've done a rough review of this patch for now. On Thursday, I’ll conduct a more detailed review since I won’t be in the office this Wednesday.
I'm not sure if this is the issue you discovered. (The main patch of this pre-commit might have been lost.)
https://reviews.llvm.org/D157375
We had an internal discussion later and found that this case should be vectorizable in the form of an AnyOf reduction.

llvm/lib/Analysis/IVDescriptors.cpp

artagnon · 2024-12-03T18:43:56Z

I'm not sure if this is the issue you discovered. (The main patch of this pre-commit might have been lost.)
https://reviews.llvm.org/D157375

Yes, it's the same issue, and there's a test for it in the tree, as the commit message mentions.

Mel-Chen

I support unification AnyOf.
But I don't think non-arithmetical reduction (min/max, AnyOf, FindLast...etc.) needs getOpcode. I think a better approach is to remove the dependence of non-arithmetical reduction on getOpcode and then start to unify AnyOf.
Here is my first step #118777.

llvm/lib/Analysis/IVDescriptors.cpp

Mel-Chen

This is not NFC since getOpcode() returned different result from before.

llvm/include/llvm/Analysis/IVDescriptors.h

Mel-Chen · 2025-01-16T11:26:17Z

@artagnon
Sorry, I had to use strong way to prevent the merge of this patch because I was out of the office yesterday and couldn't respond.

The purpose of this PR is really good, and I think it should continue, but my concern is that the approach and results are incorrect.

I believe the real reason we cannot unify IAnyOf and FAnyOf is due to getOpcode(). In vectorizer, there are still many cases where getOpcode() is called unnecessarily, even though the execution path should not require it. Generally, getOpcode() should only have a meaningful effect for arithmetic reductions.

Earlier, I identified the locations where getOpcode() is currently being called: (the line numbers might be outdated because this information has been sitting in my notebook for about a month).

/scratch/melc/llvm-project/llvm/lib/Analysis/IVDescriptors.cpp:1066:30: #118777 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:553:37
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:2133:53:
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:2144:53:
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:2181:78:
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:2193:39:
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:2231:67:
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:2253:70: 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4874:57: preferInLoopReduction should pass kind
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5860:27: 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5884:34: 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5922:27: 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5930:41: 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7026:41: 
/scratch/melc/llvm-project/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9486:57: preferPredicatedReductionSelect should pass kind
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp: 19360 [done] #122239 
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp: 20515 [look nice]

Because I've been busy with EVL tail folding, the cleanup may proceed slowly. Welcome your contribution If you’re willing and interested. Then we can continue this patch after we finish the clean up. :)

ayalz

Adding some thoughts raised by this unification effort.

llvm/include/llvm/Analysis/IVDescriptors.h

This patch changes the preferInLoopReduction function to take a RecurKind instead of an unsigned Opcode. This makes it possible to distinguish non-arithmetic reductions such as min/max, AnyOf, and FindLastIV, and also helps unify IAnyOf with FAnyOf and IFindLastIV with FFindLastIV. Related patch #118393 #131830

…ctionResult, nfc (#140245) When reducing unrolled parts, explicitly check for min/max reductions using the function RecurrenceDescriptor::isMinMaxRecurrenceKind. Only if the reduction is not min/max reduction, call RecurrenceDescriptor::getOpcode() to handle other cases via CreateBinOp. Based on #140242 Related to #118393

…ComputeReductionResult, nfc (#140245) When reducing unrolled parts, explicitly check for min/max reductions using the function RecurrenceDescriptor::isMinMaxRecurrenceKind. Only if the reduction is not min/max reduction, call RecurrenceDescriptor::getOpcode() to handle other cases via CreateBinOp. Based on llvm/llvm-project#140242 Related to llvm/llvm-project#118393

Mel-Chen · 2025-05-21T09:09:22Z

@artagnon I think you can re-open and continue this work.

artagnon · 2025-05-22T13:17:33Z

Thanks @Mel-Chen for the prerequisite work. This patch should be ready to review now.

Checking RecurKind::FAnyOf in isRecurrenceDescriptor() is wasted work when it already checks RecurKind::IAnyOf. Affect a minor adjustment to the code to facilitate skipping the RecurKind::FAnyOf check, and strip the check. The patch has the side-effect of rewriting some flaky code, which would match an ICmp having the reduction phi as an operand with IAnyOf, and due to NumCmpSelectPatternInst != 1 (the select redux is also matched), it would have to rely on failing to match an FCmp with FAnyOf, setting NumCmpSelectPatternInst = 1, and successfully vectorizing an IAnyOf pattern with the incorrect debug output. There is a test for this already in select-cmp.ll: select_i32_from_icmp_same_inputs. Co-authored-by: Mel Chen <[email protected]>

Mel-Chen

LGTM
Maybe it's worth updating the patch description?

fhahn

LGTM, thanks

Co-authored-by: Mel Chen <[email protected]>

…ctionResult, nfc (llvm#140245) When reducing unrolled parts, explicitly check for min/max reductions using the function RecurrenceDescriptor::isMinMaxRecurrenceKind. Only if the reduction is not min/max reduction, call RecurrenceDescriptor::getOpcode() to handle other cases via CreateBinOp. Based on llvm#140242 Related to llvm#118393

artagnon requested review from fhahn, Mel-Chen, davemgreen, huntergr-arm and david-arm December 2, 2024 20:10

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Dec 2, 2024

fhahn reviewed Dec 3, 2024

View reviewed changes

artagnon changed the title ~~IVDescriptors: cut wasteful FAnyOf checking (NFC)~~ IVDescriptors: cut wasteful FAnyOf checking Dec 3, 2024

artagnon force-pushed the ivdesc-fanyof-strip branch from 8ce79f5 to b70d6e3 Compare December 3, 2024 11:36

llvmbot added backend:AArch64 backend:RISC-V vectorizers llvm:transforms labels Dec 3, 2024

artagnon changed the title ~~IVDescriptors: cut wasteful FAnyOf checking~~ IVDescriptors: unify RecurKinds IAnyOf and FAnyOf Dec 3, 2024

artagnon requested a review from alexey-bataev December 3, 2024 11:40

fhahn reviewed Dec 3, 2024

View reviewed changes

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp Outdated Show resolved Hide resolved

llvm/lib/Analysis/IVDescriptors.cpp Outdated Show resolved Hide resolved

artagnon changed the title ~~IVDescriptors: unify RecurKinds IAnyOf and FAnyOf~~ IVDesc: unify RecurKinds IAnyOf and FAnyOf Dec 3, 2024

artagnon force-pushed the ivdesc-fanyof-strip branch from b70d6e3 to 05ee143 Compare December 3, 2024 15:49

Mel-Chen mentioned this pull request Dec 3, 2024

[LoopVectorize] Vectorize select-cmp reduction pattern for increasing integer induction variable #67812

Merged

Mel-Chen reviewed Dec 3, 2024

View reviewed changes

llvm/lib/Analysis/IVDescriptors.cpp Show resolved Hide resolved

llvm/lib/Analysis/IVDescriptors.cpp Outdated Show resolved Hide resolved

Mel-Chen reviewed Dec 5, 2024

View reviewed changes

llvm/lib/Analysis/IVDescriptors.cpp Outdated Show resolved Hide resolved

Mel-Chen reviewed Dec 9, 2024

View reviewed changes

llvm/lib/Analysis/IVDescriptors.cpp Outdated Show resolved Hide resolved

artagnon force-pushed the ivdesc-fanyof-strip branch from 71d0bc8 to 2c387ce Compare December 16, 2024 10:43

Mel-Chen requested changes Jan 14, 2025

View reviewed changes

llvm/include/llvm/Analysis/IVDescriptors.h Outdated Show resolved Hide resolved

artagnon closed this Jan 14, 2025

artagnon deleted the ivdesc-fanyof-strip branch January 14, 2025 16:54

artagnon restored the ivdesc-fanyof-strip branch January 16, 2025 12:26

artagnon reopened this Jan 16, 2025

artagnon marked this pull request as draft January 16, 2025 12:29

ayalz reviewed Jan 20, 2025

View reviewed changes

llvm/include/llvm/Analysis/IVDescriptors.h Outdated Show resolved Hide resolved

llvm/include/llvm/Analysis/IVDescriptors.h Outdated Show resolved Hide resolved

Mel-Chen mentioned this pull request Mar 24, 2025

[TTI][LV] Change the prototype of preferInLoopReduction. nfc #132698

Merged

Mel-Chen mentioned this pull request May 16, 2025

[VPlan] Clean up the function VPInstruction::generate for ComputeReductionResult, nfc #140245

Merged

artagnon force-pushed the ivdesc-fanyof-strip branch from af86167 to c9b44e9 Compare May 22, 2025 13:16

artagnon marked this pull request as ready for review May 22, 2025 13:16

artagnon changed the title ~~IVDesc: unify RecurKinds IAnyOf and FAnyOf~~ [IVDesc] Unify RecurKinds IAnyOf and FAnyOf May 22, 2025

artagnon changed the title ~~[IVDesc] Unify RecurKinds IAnyOf and FAnyOf~~ [IVDesc] Unify RecurKinds [I|F]AnyOf (NFC) May 22, 2025

artagnon changed the title ~~[IVDesc] Unify RecurKinds [I|F]AnyOf (NFC)~~ [IVDesc] Unify RecurKinds [I|F]AnyOf May 22, 2025

artagnon force-pushed the ivdesc-fanyof-strip branch from c740737 to 89e1f95 Compare May 22, 2025 18:16

artagnon force-pushed the ivdesc-fanyof-strip branch from 89e1f95 to 7ddb7dd Compare May 23, 2025 08:30

Mel-Chen approved these changes May 23, 2025

View reviewed changes

fhahn approved these changes May 23, 2025

View reviewed changes

artagnon merged commit 0240129 into llvm:main May 23, 2025
11 checks passed

artagnon deleted the ivdesc-fanyof-strip branch May 23, 2025 10:57

sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025

[IVDesc] Unify RecurKinds [I|F]AnyOf (llvm#118393)

6aa878b

Co-authored-by: Mel Chen <[email protected]>

[IVDesc] Unify RecurKinds [I|F]AnyOf #118393

[IVDesc] Unify RecurKinds [I|F]AnyOf #118393

Uh oh!

Conversation

artagnon commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

artagnon commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-arm commented Dec 3, 2024

Uh oh!

david-arm commented Dec 3, 2024

Uh oh!

artagnon commented Dec 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artagnon commented Dec 3, 2024

Uh oh!

Mel-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

artagnon commented Dec 3, 2024

Uh oh!

Mel-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mel-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mel-Chen commented Jan 16, 2025

Uh oh!

ayalz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mel-Chen commented May 21, 2025

Uh oh!

artagnon commented May 22, 2025

Uh oh!

Mel-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

artagnon commented Dec 2, 2024 •

edited

Loading

llvmbot commented Dec 2, 2024 •

edited

Loading

artagnon commented Dec 3, 2024 •

edited

Loading