[llvm] 47f9109 - [InstCombine] Guard against many users when swapping icmp operands

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2023-02-27T12:12:33+01:00
New Revision: 47f9109dff80a1abbe2705ee71dc0882b1d62274

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

LOG: [InstCombine] Guard against many users when swapping icmp operands

This addresses the compile-time regression reported on D144369.
If we don't fold constant operands early, then we might end up
walking very large use lists of constants here. Explicitly exclude
constants, and also limit the number of inspected users to avoid
degenerate cases like this.

This entire transform shouldn't be part of InstCombine in the
first place though.

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index b2ed32b129e81..c8c24c75f655b 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -5455,17 +5455,22 @@ static APInt getDemandedBitsLHSMask(ICmpInst &I, unsigned BitWidth) {
 /// The rationale is that several architectures use the same instruction for
 /// both subtract and cmp. Thus, it is better if the order of those operands
 /// match.
+/// TODO: Shouldn't this be part of CGP instead?
 /// \return true if Op0 and Op1 should be swapped.
 static bool swapMayExposeCSEOpportunities(const Value *Op0, const Value *Op1) {
   // Filter out pointer values as those cannot appear directly in subtract.
   // FIXME: we may want to go through inttoptrs or bitcasts.
-  if (Op0->getType()->isPointerTy())
+  if (Op0->getType()->isPointerTy() || isa<Constant>(Op0))
     return false;
   // If a subtract already has the same operands as a compare, swapping would be
   // bad. If a subtract has the same operands as a compare but in reverse order,
   // then swapping is good.
   int GoodToSwap = 0;
+  unsigned NumInspected = 0;
   for (const User *U : Op0->users()) {
+    // Avoid walking many users.
+    if (++NumInspected > 128)
+      return false;
     if (match(U, m_Sub(m_Specific(Op1), m_Specific(Op0))))
       GoodToSwap++;
     else if (match(U, m_Sub(m_Specific(Op0), m_Specific(Op1))))