[ValueTracking] Compute KnownFP state from recursive select/phi. #113686

davemgreen · 2024-10-25T13:40:54Z

Given a recursive phi with select:
%p = phi [ 0, entry ], [ %sel, loop]
%sel = select %c, %other, %p

The fp state can be calculated using the knowledge that the select/phi pair can only be the initial state (0 here) or from %other. This adds a short-cut into computeKnownFPClass for PHI to detect that the select is recursive back to the phi, and if so use the state from the other operand.

This helps to address a regression from #83200.

llvmbot · 2024-10-25T13:41:27Z

@llvm/pr-subscribers-llvm-transforms

Author: David Green (davemgreen)

Changes

Given a recursive phi with select:
%p = phi [ 0, entry ], [ %sel, loop]
%sel = select %c, %other, %p

The fp state can be calculated using the knowledge that the select/phi pair can only be the initial state (0 here) or from %other. This adds a short-cut into computeKnownFPClass for PHI to detect that the select is recursive back to the phi, and if so use the state from the other operand.

This helps to address a regression from #83200.

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

2 Files Affected:

(modified) llvm/lib/Analysis/ValueTracking.cpp (+7)
(modified) llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll (+2-2)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index e9ed8b3c862b55..708922e21ee0bb 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -5999,6 +5999,13 @@ void computeKnownFPClass(const Value *V, const APInt &DemandedElts,
         if (IncValue == P)
           continue;
 
+        // If the Use is a select of this phi, use the fp class of the other
+        // operand to break the recursion.
+        Value *V;
+        if (match(IncValue, m_Select(m_Value(), m_Specific(P), m_Value(V))) ||
+            match(IncValue, m_Select(m_Value(), m_Value(V), m_Specific(P))))
+          IncValue = V;
+
         KnownFPClass KnownSrc;
         // Recurse, but cap the recursion to two levels, because we don't want
         // to waste time spinning around in loops. We need at least depth 2 to
diff --git a/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll b/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll
index fc56754166d609..b54d4a97d07be7 100644
--- a/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll
+++ b/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll
@@ -45,8 +45,8 @@ define float @test(ptr nocapture readonly %pA, ptr nocapture readonly %pB, i32 %
 ; CHECK-NEXT:    [[TMP7:%.*]] = fsub fast <4 x float> [[WIDE_LOAD]], [[WIDE_LOAD7]]
 ; CHECK-NEXT:    [[TMP8:%.*]] = call fast <4 x float> @llvm.fabs.v4f32(<4 x float> [[TMP7]])
 ; CHECK-NEXT:    [[TMP9:%.*]] = fdiv fast <4 x float> [[TMP8]], [[TMP6]]
-; CHECK-NEXT:    [[TMP10:%.*]] = fadd fast <4 x float> [[TMP9]], [[VEC_PHI]]
-; CHECK-NEXT:    [[PREDPHI]] = select <4 x i1> [[DOTNOT9]], <4 x float> [[VEC_PHI]], <4 x float> [[TMP10]]
+; CHECK-NEXT:    [[TMP10:%.*]] = select <4 x i1> [[DOTNOT9]], <4 x float> <float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00>, <4 x float> [[TMP9]]
+; CHECK-NEXT:    [[PREDPHI]] = fadd fast <4 x float> [[VEC_PHI]], [[TMP10]]
 ; CHECK-NEXT:    [[INDEX_NEXT]] = add nuw i32 [[INDEX]], 4
 ; CHECK-NEXT:    [[TMP11:%.*]] = icmp eq i32 [[INDEX_NEXT]], [[N_VEC]]
 ; CHECK-NEXT:    br i1 [[TMP11]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop [[LOOP0:![0-9]+]]

llvmbot · 2024-10-25T13:41:28Z

@llvm/pr-subscribers-llvm-analysis

Author: David Green (davemgreen)

Changes

Given a recursive phi with select:
%p = phi [ 0, entry ], [ %sel, loop]
%sel = select %c, %other, %p

The fp state can be calculated using the knowledge that the select/phi pair can only be the initial state (0 here) or from %other. This adds a short-cut into computeKnownFPClass for PHI to detect that the select is recursive back to the phi, and if so use the state from the other operand.

This helps to address a regression from #83200.

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

2 Files Affected:

(modified) llvm/lib/Analysis/ValueTracking.cpp (+7)
(modified) llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll (+2-2)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index e9ed8b3c862b55..708922e21ee0bb 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -5999,6 +5999,13 @@ void computeKnownFPClass(const Value *V, const APInt &DemandedElts,
         if (IncValue == P)
           continue;
 
+        // If the Use is a select of this phi, use the fp class of the other
+        // operand to break the recursion.
+        Value *V;
+        if (match(IncValue, m_Select(m_Value(), m_Specific(P), m_Value(V))) ||
+            match(IncValue, m_Select(m_Value(), m_Value(V), m_Specific(P))))
+          IncValue = V;
+
         KnownFPClass KnownSrc;
         // Recurse, but cap the recursion to two levels, because we don't want
         // to waste time spinning around in loops. We need at least depth 2 to
diff --git a/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll b/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll
index fc56754166d609..b54d4a97d07be7 100644
--- a/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll
+++ b/llvm/test/Transforms/LoopVectorize/ARM/mve-selectandorcost.ll
@@ -45,8 +45,8 @@ define float @test(ptr nocapture readonly %pA, ptr nocapture readonly %pB, i32 %
 ; CHECK-NEXT:    [[TMP7:%.*]] = fsub fast <4 x float> [[WIDE_LOAD]], [[WIDE_LOAD7]]
 ; CHECK-NEXT:    [[TMP8:%.*]] = call fast <4 x float> @llvm.fabs.v4f32(<4 x float> [[TMP7]])
 ; CHECK-NEXT:    [[TMP9:%.*]] = fdiv fast <4 x float> [[TMP8]], [[TMP6]]
-; CHECK-NEXT:    [[TMP10:%.*]] = fadd fast <4 x float> [[TMP9]], [[VEC_PHI]]
-; CHECK-NEXT:    [[PREDPHI]] = select <4 x i1> [[DOTNOT9]], <4 x float> [[VEC_PHI]], <4 x float> [[TMP10]]
+; CHECK-NEXT:    [[TMP10:%.*]] = select <4 x i1> [[DOTNOT9]], <4 x float> <float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00>, <4 x float> [[TMP9]]
+; CHECK-NEXT:    [[PREDPHI]] = fadd fast <4 x float> [[VEC_PHI]], [[TMP10]]
 ; CHECK-NEXT:    [[INDEX_NEXT]] = add nuw i32 [[INDEX]], 4
 ; CHECK-NEXT:    [[TMP11:%.*]] = icmp eq i32 [[INDEX_NEXT]], [[N_VEC]]
 ; CHECK-NEXT:    br i1 [[TMP11]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop [[LOOP0:![0-9]+]]

pawosm-arm

Tested locally, this indeed restores the lost transformation.

dtcxzyw · 2024-10-25T15:04:04Z

I think this approach is also applicable to computeKnownBits. I'll post a patch later :)

davemgreen · 2024-10-25T15:17:43Z

I think this approach is also applicable to computeKnownBits. I'll post a patch later :)

Nice, thanks! I had seen known bits had some handling of recurrences already, they didn't necessarily include selects.
I think we can expand this to phi+phi for the fpclass case too relatively easily, which will help in more complex cases.

I rebased over 577c7dd, which added a phase-ordering test.

arsenm · 2024-10-25T22:54:11Z

As far as the regression on #83200 is concerned, I think it needs a local flag handling fix

arsenm

Needs a more targeted, dedicated test

davemgreen · 2024-10-27T19:56:30Z

As far as the regression on #83200 is concerned, I think it needs a local flag handling fix

Yeah there are probably a few ways to fix it, selects are often missing fmfs due to them coming from control flow. This seemed generally useful.

Needs a more targeted, dedicated test

I've tried to add some Attributor tests, which I see have been used to test similar patches in the past. Let me know if there is a better way.

As a follow-on to llvm#113686, this breaks the recursion between phi nodes that have p1 = phi(x, p2) and p2 = phi(y, p1). The knownFPClass can be calculated from the classes of p1 and p2.

goldsteinn · 2024-10-30T14:17:25Z

llvm/lib/Analysis/ValueTracking.cpp

+        // operand to break the recursion.
+        Value *V;
+        if (match(IncValue, m_Select(m_Value(), m_Specific(P), m_Value(V))) ||
+            match(IncValue, m_Select(m_Value(), m_Value(V), m_Specific(P))))


Just a nit, but maybe this means we should add m_c_Select matcher.

It would be hard to represent that with the negated condition I think

Agreed, think it's probably not practical

arsenm · 2024-10-30T19:06:28Z

llvm/test/Transforms/Attributor/nofpclass-phiselect.ll

@@ -2,7 +2,7 @@
 ; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -S < %s | FileCheck %s

 define float @phi_select(i1 %c, float nofpclass(inf) %base, float nofpclass(inf) %arg) {


Needs another case in the other select operand?

Given a recursive phi with select: %p = phi [ 0, entry ], [ %sel, loop] %sel = select %c, %other, %p The fp state can be calculated using the knowledge that the select/phi pair can only be the initial state (0 here) or from %other. This adds a short-cut into computeKnownFPClass for PHI to detect that the select is recursive back to the phi, and if so use the state from the other operand. This helps to address a regression from llvm#83200.

As a follow-on to llvm#113686, this breaks the recursion between phi nodes that have p1 = phi(x, p2) and p2 = phi(y, p1). The knownFPClass can be calculated from the classes of p1 and p2.

This patch is inspired by #113686. I found that it removes a lot of unnecessary "and X, 1" in some applications that represent boolean values with int.

…m#113686) Given a recursive phi with select: %p = phi [ 0, entry ], [ %sel, loop] %sel = select %c, %other, %p The fp state can be calculated using the knowledge that the select/phi pair can only be the initial state (0 here) or from %other. This adds a short-cut into computeKnownFPClass for PHI to detect that the select is recursive back to the phi, and if so use the state from the other operand. This helps to address a regression from llvm#83200.

…3707) This patch is inspired by llvm#113686. I found that it removes a lot of unnecessary "and X, 1" in some applications that represent boolean values with int.

…m#113686) Given a recursive phi with select: %p = phi [ 0, entry ], [ %sel, loop] %sel = select %c, %other, %p The fp state can be calculated using the knowledge that the select/phi pair can only be the initial state (0 here) or from %other. This adds a short-cut into computeKnownFPClass for PHI to detect that the select is recursive back to the phi, and if so use the state from the other operand. This helps to address a regression from llvm#83200.

…3707) This patch is inspired by llvm#113686. I found that it removes a lot of unnecessary "and X, 1" in some applications that represent boolean values with int.

davemgreen requested review from arsenm and pawosm-arm October 25, 2024 13:40

davemgreen requested a review from nikic as a code owner October 25, 2024 13:40

llvmbot added llvm:analysis llvm:transforms labels Oct 25, 2024

This was referenced Oct 25, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

pre-commit: PR113686 dtcxzyw/llvm-opt-benchmark#1534

Closed

pawosm-arm approved these changes Oct 25, 2024

View reviewed changes

davemgreen force-pushed the gh-vt-knownfpselectphi branch from 411c89e to 4d32839 Compare October 25, 2024 14:21

dtcxzyw mentioned this pull request Oct 25, 2024

[ValueTracking] Compute known bits from recursive select/phi #113707

Merged

arsenm reviewed Oct 25, 2024

View reviewed changes

davemgreen force-pushed the gh-vt-knownfpselectphi branch from 4d32839 to b17b64b Compare October 27, 2024 19:56

davemgreen mentioned this pull request Oct 29, 2024

[ValueTracking] Handle recursive phis in knownFPClass #114008

Merged

davemgreen force-pushed the gh-vt-knownfpselectphi branch from b17b64b to 7cbb47e Compare October 30, 2024 08:12

goldsteinn reviewed Oct 30, 2024

View reviewed changes

arsenm reviewed Oct 30, 2024

View reviewed changes

davemgreen added 2 commits October 30, 2024 23:45

Add an extra commuted select test.

54efb00

davemgreen force-pushed the gh-vt-knownfpselectphi branch from 7cbb47e to 54efb00 Compare October 31, 2024 00:07

arsenm approved these changes Oct 31, 2024

View reviewed changes

davemgreen merged commit 9735c05 into llvm:main Oct 31, 2024
8 checks passed

davemgreen deleted the gh-vt-knownfpselectphi branch October 31, 2024 07:50

davemgreen mentioned this pull request Nov 5, 2024

[Transforms][Utils][PromoteMem2Reg] Propagate nnan and ninf flags on par with the nsz flag #114271

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ValueTracking] Compute KnownFP state from recursive select/phi. #113686

[ValueTracking] Compute KnownFP state from recursive select/phi. #113686

davemgreen commented Oct 25, 2024

llvmbot commented Oct 25, 2024

llvmbot commented Oct 25, 2024

pawosm-arm left a comment

dtcxzyw commented Oct 25, 2024

davemgreen commented Oct 25, 2024

arsenm commented Oct 25, 2024

arsenm left a comment

davemgreen commented Oct 27, 2024

goldsteinn Oct 30, 2024

arsenm Oct 30, 2024

goldsteinn Oct 31, 2024

arsenm Oct 30, 2024

		@@ -2,7 +2,7 @@
		; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -S < %s \| FileCheck %s

		define float @phi_select(i1 %c, float nofpclass(inf) %base, float nofpclass(inf) %arg) {

[ValueTracking] Compute KnownFP state from recursive select/phi. #113686

[ValueTracking] Compute KnownFP state from recursive select/phi. #113686

Conversation

davemgreen commented Oct 25, 2024

llvmbot commented Oct 25, 2024

llvmbot commented Oct 25, 2024

pawosm-arm left a comment

Choose a reason for hiding this comment

dtcxzyw commented Oct 25, 2024

davemgreen commented Oct 25, 2024

arsenm commented Oct 25, 2024

arsenm left a comment

Choose a reason for hiding this comment

davemgreen commented Oct 27, 2024

goldsteinn Oct 30, 2024

Choose a reason for hiding this comment

arsenm Oct 30, 2024

Choose a reason for hiding this comment

goldsteinn Oct 31, 2024

Choose a reason for hiding this comment

arsenm Oct 30, 2024

Choose a reason for hiding this comment