[PATCH] D127903: [InstCombine] Optimize test for same-sign of values

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I committed the tests with baseline results, so we have those in place independently of the code change. I then updated the tests to provide a bit more coverage. Please rebase/update after:
feb4b336acc71 <https://reviews.llvm.org/rGfeb4b336acc7100fbb87fdd8f61f1d58cd234709>

It's programmer preference, but I think it'd be easier to read with less IsAnd ? () :() constructs, so something like this:

  diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  index cc06e2fd45db..4f8814475e12 100644
  --- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  +++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  @@ -2647,6 +2647,38 @@ Value *InstCombinerImpl::foldAndOrOfICmps(ICmpInst *LHS, ICmpInst *RHS,
       }
     }
   
  +  // Match naive pattern (and its inverted form) for checking if two values
  +  // share same sign. An example of the pattern: (icmp slt (X & Y), 0) | (icmp
  +  // sgt (X | Y), -1) -> (icmp sgt (X ^ Y), -1) Inverted form (example): (icmp
  +  // slt (X | Y), 0) & (icmp sgt (X & Y), -1) -> (icmp slt (X ^ Y), 0)
  +  bool TrueIfSignedL, TrueIfSignedR;
  +  if (InstCombiner::isSignBitCheck(PredL, *LHSC, TrueIfSignedL) &&
  +      InstCombiner::isSignBitCheck(PredR, *RHSC, TrueIfSignedR) &&
  +      (RHS->hasOneUse() || LHS->hasOneUse())) {
  +    Value *X, *Y;
  +    if (IsAnd) {
  +      if ((TrueIfSignedL && !TrueIfSignedR &&
  +           match(LHS0, m_Or(m_Value(X), m_Value(Y))) &&
  +           match(RHS0, m_c_And(m_Specific(X), m_Specific(Y)))) ||
  +          (!TrueIfSignedL && TrueIfSignedR &&
  +           match(LHS0, m_And(m_Value(X), m_Value(Y))) &&
  +           match(RHS0, m_c_Or(m_Specific(X), m_Specific(Y))))) {
  +        Value *NewXor = Builder.CreateXor(X, Y);
  +        return Builder.CreateIsNeg(NewXor);
  +      }
  +    } else {
  +      if (((TrueIfSignedL && !TrueIfSignedR &&
  +            match(LHS0, m_And(m_Value(X), m_Value(Y))) &&
  +            match(RHS0, m_c_Or(m_Specific(X), m_Specific(Y))))) ||
  +          (!TrueIfSignedL && TrueIfSignedR &&
  +           match(LHS0, m_Or(m_Value(X), m_Value(Y))) &&
  +           match(RHS0, m_c_And(m_Specific(X), m_Specific(Y))))) {
  +        Value *NewXor = Builder.CreateXor(X, Y);
  +        return Builder.CreateIsNotNeg(NewXor);
  +      }
  +    }
  +  }
  +
     return foldAndOrOfICmpsUsingRanges(LHS, RHS, IsAnd);
   }
   



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2640
+      InstCombiner::isSignBitCheck(PredR, *RHSC, TrueIfSignedR) &&
+      RHS->hasOneUse() && LHS->hasOneUse()) {
+    Value *X, *Y;
----------------
The use checks are too strict. This optimization should be a win as long as any one of the intermediate values has one use. We are creating 2 new values, so as long as we can remove one of the old ones + the final value, it is an improvement.

I'd make this:
  (LHS->hasOneUse() || RHS->hasOneUse())
..and remove the use checks for LHS0 and RHS0 under here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2654-2658
+      Value *NewC = IsAnd ? ConstantInt::getNullValue(NewXor->getType())
+                          : ConstantInt::getAllOnesValue(NewXor->getType());
+      ICmpInst::Predicate NewPred =
+          IsAnd ? ICmpInst::ICMP_SLT : ICmpInst::ICMP_SGT;
+      return Builder.CreateICmp(NewPred, NewXor, NewC);
----------------
There's a Builder shortcut to create the new ICmp: `CreateIsNeg` / `CreateIsNotNeg`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127903/new/

https://reviews.llvm.org/D127903