Skip to content

Conversation

@nikic
Copy link
Contributor

@nikic nikic commented Nov 5, 2024

As discussed on #114689 (review) and following, there is no principled reason why the phi of select case should have a different recursion limit than the general case. There may still be fan-out, and there may still be indirect recursion. Revert that part of #113707.

As discussed on llvm#114689 (review) and following, there is no principled reason why
the phi of select case should have a different recursion limit
than the general case. There may still be fan-out, and there may
still be indirect recursion. Revert that part of llvm#113707.
@nikic nikic requested a review from dtcxzyw November 5, 2024 14:21
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Nov 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 5, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Nikita Popov (nikic)

Changes

As discussed on #114689 (review) and following, there is no principled reason why the phi of select case should have a different recursion limit than the general case. There may still be fan-out, and there may still be indirect recursion. Revert that part of #113707.


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

1 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+8-10)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 5c20c24d0ae00a..7d8e0d65dbfebb 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1566,20 +1566,12 @@ static void computeKnownBitsFromOperator(const Operator *I,
         // Skip direct self references.
         if (IncValue == P) continue;
 
-        // Recurse, but cap the recursion to one level, because we don't
-        // want to waste time spinning around in loops.
-        // TODO: See if we can base recursion limiter on number of incoming phi
-        // edges so we don't overly clamp analysis.
-        unsigned IncDepth = MaxAnalysisRecursionDepth - 1;
-
         // If the Use is a select of this phi, use the knownbit of the other
         // operand to break the recursion.
         if (auto *SI = dyn_cast<SelectInst>(IncValue)) {
-          if (SI->getTrueValue() == P || SI->getFalseValue() == P) {
+          if (SI->getTrueValue() == P || SI->getFalseValue() == P)
             IncValue = SI->getTrueValue() == P ? SI->getFalseValue()
                                                : SI->getTrueValue();
-            IncDepth = Depth + 1;
-          }
         }
 
         // Change the context instruction to the "edge" that flows into the
@@ -1590,7 +1582,13 @@ static void computeKnownBitsFromOperator(const Operator *I,
         RecQ.CxtI = P->getIncomingBlock(u)->getTerminator();
 
         Known2 = KnownBits(BitWidth);
-        computeKnownBits(IncValue, DemandedElts, Known2, IncDepth, RecQ);
+
+        // Recurse, but cap the recursion to one level, because we don't
+        // want to waste time spinning around in loops.
+        // TODO: See if we can base recursion limiter on number of incoming phi
+        // edges so we don't overly clamp analysis.
+        computeKnownBits(IncValue, DemandedElts, Known2,
+                         MaxAnalysisRecursionDepth - 1, RecQ);
 
         // See if we can further use a conditional branch into the phi
         // to help us determine the range of the value.

@nikic nikic merged commit 2d7f34f into llvm:main Nov 7, 2024
8 of 10 checks passed
@nikic nikic deleted the instcombine-known-bits-phi-sel branch November 7, 2024 09:14
banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Nov 7, 2024
As discussed on
llvm#114689 (review)
and following, there is no principled reason why the phi of select case
should have a different recursion limit than the general case. There may
still be fan-out, and there may still be indirect recursion. Revert that
part of llvm#113707.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:analysis Includes value tracking, cost tables and constant folding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants