[PATCH] D49238: [InstCombine] add more SPFofSPF folding

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I think the logic is correct, but it would be easier to read if we re-organized it like this:

  Index: lib/Analysis/ValueTracking.cpp
  ===================================================================
  --- lib/Analysis/ValueTracking.cpp	(revision 336964)
  +++ lib/Analysis/ValueTracking.cpp	(working copy)
  @@ -4627,34 +4627,49 @@
       }
     }
   
  -  // Sign-extending LHS does not change its sign, so TrueVal/FalseVal can
  -  // match against either LHS or sext(LHS).
  -  auto MaybeSExtLHS = m_CombineOr(m_Specific(CmpLHS),
  -                                  m_SExt(m_Specific(CmpLHS)));
  -  if ((match(TrueVal, MaybeSExtLHS) &&
  -       match(FalseVal, m_Neg(m_Specific(TrueVal)))) ||
  -      (match(FalseVal, MaybeSExtLHS) &&
  -       match(TrueVal, m_Neg(m_Specific(FalseVal))))) {
  -    // Set LHS and RHS so that RHS is the negated operand of the select
  -    if (match(TrueVal, MaybeSExtLHS)) {
  +  if (isKnownNegation(TrueVal, FalseVal)) {
  +    // Sign-extending LHS does not change its sign, so TrueVal/FalseVal can
  +    // match against either LHS or sext(LHS).
  +    auto MaybeSExtCmpLHS = m_CombineOr(m_Specific(CmpLHS),
  +                                       m_SExt(m_Specific(CmpLHS)));
  +    auto ZeroOrAllOnes = m_CombineOr(m_ZeroInt(), m_AllOnes());
  +    auto ZeroOrOne = m_CombineOr(m_ZeroInt(), m_One());
  +    if (match(TrueVal, MaybeSExtCmpLHS)) {
  +      // Set the return values. If the compare uses the negated value (-X >s 0),
  +      // swap the return values because the negated value is always 'RHS'.
         LHS = TrueVal;
         RHS = FalseVal;
  -    } else {
  +      if (match(CmpLHS, m_Neg(m_Specific(FalseVal))))
  +        std::swap(LHS, RHS);
  +
  +      // (X >s 0) ? X : -X or (X >s -1) ? X : -X --> ABS(X)
  +      // (-X >s 0) ? -X : X or (-X >s -1) ? -X : X --> ABS(X)
  +      if (Pred == ICmpInst::ICMP_SGT && match(CmpRHS, ZeroOrAllOnes))
  +        return {SPF_ABS, SPNB_NA, false};
  +
  +      // (X <s 0) ? X : -X or (X <s 1) ? X : -X --> NABS(X)
  +      // (-X <s 0) ? -X : X or (-X <s 1) ? -X : X --> NABS(X)
  +      if (Pred == ICmpInst::ICMP_SLT && match(CmpRHS, ZeroOrOne))
  +        return {SPF_NABS, SPNB_NA, false};
  +    }
  +    if (match(FalseVal, MaybeSExtCmpLHS)) {
  +      // Set the return values. If the compare uses the negated value (-X >s 0),
  +      // swap the return values because the negated value is always 'RHS'.
         LHS = FalseVal;
         RHS = TrueVal;
  -    }
  +      if (match(CmpLHS, m_Neg(m_Specific(TrueVal))))
  +        std::swap(LHS, RHS);
   
  -    // (X >s 0) ? X : -X or (X >s -1) ? X : -X --> ABS(X)
  -    // (X >s 0) ? -X : X or (X >s -1) ? -X : X --> NABS(X)
  -    if (Pred == ICmpInst::ICMP_SGT &&
  -        match(CmpRHS, m_CombineOr(m_ZeroInt(), m_AllOnes())))
  -      return {(LHS == TrueVal) ? SPF_ABS : SPF_NABS, SPNB_NA, false};
  +      // (X >s 0) ? -X : X or (X >s -1) ? -X : X --> NABS(X)
  +      // (-X >s 0) ? X : -X or (-X >s -1) ? X : -X --> NABS(X)
  +      if (Pred == ICmpInst::ICMP_SGT && match(CmpRHS, ZeroOrAllOnes))
  +        return {SPF_NABS, SPNB_NA, false};
   
  -    // (X <s 0) ? -X : X or (X <s 1) ? -X : X --> ABS(X)
  -    // (X <s 0) ? X : -X or (X <s 1) ? X : -X --> NABS(X)
  -    if (Pred == ICmpInst::ICMP_SLT &&
  -        match(CmpRHS, m_CombineOr(m_ZeroInt(), m_One())))
  -      return {(LHS == FalseVal) ? SPF_ABS : SPF_NABS, SPNB_NA, false};
  +      // (X <s 0) ? -X : X or (X <s 1) ? -X : X --> ABS(X)
  +      // (-X <s 0) ? X : -X or (-X <s 1) ? X : -X --> ABS(X)
  +      if (Pred == ICmpInst::ICMP_SLT && match(CmpRHS, ZeroOrOne))
  +        return {SPF_ABS, SPNB_NA, false};
  +    }
     }
   
     if (CmpInst::isIntPredicate(Pred))


https://reviews.llvm.org/D49238