Fix m_[Ord|Unord][FMin|FMax] matchers to correctly match ordering.

[a.elovikov]

Summary

Previously, the matching was done incorrectly for the case where
operands for FCmpInst and SelectInst were in opposite order.

[a.elovikov]

Ping.

[spatel]

Sorry for the delay. Can this bug be exposed with an IR transform or a codegen test?

[a.elovikov]

In D33185#778032, @spatel wrote:

Sorry for the delay. Can this bug be exposed with an IR transform or a codegen test?

I am not aware of such cases. I've faced that when working on D33186 but later decided to work with fast math only there. But I believe that this is still useful to fix. At least someone else won't be surprised by it.

[spatel]

In D33185#778060, @a.elovikov wrote:

In D33185#778032, @spatel wrote:

Sorry for the delay. Can this bug be exposed with an IR transform or a codegen test?

I am not aware of such cases. I've faced that when working on D33186 but later decided to work with fast math only there. But I believe that this is still useful to fix. At least someone else won't be surprised by it.

Agreed - we need to fix the bug. LGTM. Please see inline comments too for some cleanup.

llvm/include/llvm/IR/PatternMatch.h

1141

max(L, R) --> min(L, R)

1157

m_UnordFMin(L, R) --> m_UnordFMax(L, R)

1224–1225

max(L, R) --> min(L, R)

1224–1225

This should be moved up with the related fmin/fmax templates.

[a.elovikov]

• Fix typos in comments/move m_UnordFMin function as asked.

[spatel]

Thanks! Do you need someone to commit this for you?

[craig.topper]

@spatel Yes he does. He sent me a mail asking for me to commit it for him. Do you want to do it or do you want me to?

[spatel]

In D33185#779080, @craig.topper wrote:

@spatel Yes he does. He sent me a mail asking for me to commit it for him. Do you want to do it or do you want me to?

Please go ahead if you have that queued up.

llvm/include/llvm/IR/PatternMatch.h

template <typename OpTy> bool match(OpTy *V) {
     Value *TrueVal = SI->getTrueValue();
     Value *FalseVal = SI->getFalseValue();
     Value *LHS = Cmp->getOperand(0);
     Value *RHS = Cmp->getOperand(1);
     if ((TrueVal != LHS || FalseVal != RHS) &&
         (TrueVal != RHS || FalseVal != LHS))
       return false;
     typename CmpInst_t::Predicate Pred =
-        LHS == TrueVal ? Cmp->getPredicate() : Cmp->getSwappedPredicate();
+        LHS == TrueVal ? Cmp->getPredicate() : Cmp->getInversePredicate();
     // Does "(x pred y) ? x : y" represent the desired max/min operation?
     if (!Pred_t::match(Pred))
       return false;
     // It does!  Bind the operands.
     return L.match(LHS) && R.match(RHS);
   }
 };
 

 
 /// \brief Match an 'ordered' floating point minimum function.
 /// Floating point has one special value 'NaN'. Therefore, there is no total
 /// order. However, if we can ignore the 'NaN' value (for example, because of a
 /// 'no-nans-float-math' flag) a combination of a fcmp and select has 'minimum'
 /// semantics. In the presence of 'NaN' we have to preserve the original
 /// select(fcmp(olt/le, L, R), L, R) semantics matched by this predicate.
 ///
-///                         max(L, R)  iff L and R are not NaN
+///                         min(L, R)  iff L and R are not NaN
 ///  m_OrdFMin(L, R) =      R          iff L or R are NaN
 template <typename LHS, typename RHS>
 inline MaxMin_match<FCmpInst, LHS, RHS, ofmin_pred_ty> m_OrdFMin(const LHS &L,
                                                                  const RHS &R) {
   return MaxMin_match<FCmpInst, LHS, RHS, ofmin_pred_ty>(L, R);
 }
 
 /// \brief Match an 'unordered' floating point maximum function.
 /// Floating point has one special value 'NaN'. Therefore, there is no total
 /// order. However, if we can ignore the 'NaN' value (for example, because of a
 /// 'no-nans-float-math' flag) a combination of a fcmp and select has 'maximum'
 /// semantics. In the presence of 'NaN' we have to preserve the original
 /// select(fcmp(ugt/ge, L, R), L, R) semantics matched by this predicate.
 ///
 ///                         max(L, R)  iff L and R are not NaN
-///  m_UnordFMin(L, R) =    L          iff L or R are NaN
+///  m_UnordFMax(L, R) =    L          iff L or R are NaN
 template <typename LHS, typename RHS>
 inline MaxMin_match<FCmpInst, LHS, RHS, ufmax_pred_ty>
 m_UnordFMax(const LHS &L, const RHS &R) {
   return MaxMin_match<FCmpInst, LHS, RHS, ufmax_pred_ty>(L, R);
 }
 
+/// \brief Match an 'unordered' floating point minimum function.
+/// Floating point has one special value 'NaN'. Therefore, there is no total
+/// order. However, if we can ignore the 'NaN' value (for example, because of a
+/// 'no-nans-float-math' flag) a combination of a fcmp and select has 'minimum'
+/// semantics. In the presence of 'NaN' we have to preserve the original
+/// select(fcmp(ult/le, L, R), L, R) semantics matched by this predicate.
+///
+///                          min(L, R)  iff L and R are not NaN
+///  m_UnordFMin(L, R) =     L          iff L or R are NaN
+template <typename LHS, typename RHS>
+inline MaxMin_match<FCmpInst, LHS, RHS, ufmin_pred_ty>
+m_UnordFMin(const LHS &L, const RHS &R) {
+  return MaxMin_match<FCmpInst, LHS, RHS, ufmin_pred_ty>(L, R);
+}
+
 //===----------------------------------------------------------------------===//
 // Matchers for overflow check patterns: e.g. (a + b) u< a
 //
 
 template <typename LHS_t, typename RHS_t, typename Sum_t>
 struct UAddWithOverflow_match {
   LHS_t L;
   RHS_t R;

 ///
 /// S is matched to the addition whose result is being checked for overflow, and
 /// L and R are matched to the LHS and RHS of S.
 template <typename LHS_t, typename RHS_t, typename Sum_t>
 UAddWithOverflow_match<LHS_t, RHS_t, Sum_t>
 m_UAddWithOverflow(const LHS_t &L, const RHS_t &R, const Sum_t &S) {
   return UAddWithOverflow_match<LHS_t, RHS_t, Sum_t>(L, R, S);
 }
 
-/// \brief Match an 'unordered' floating point minimum function.
-/// Floating point has one special value 'NaN'. Therefore, there is no total
-/// order. However, if we can ignore the 'NaN' value (for example, because of a
-/// 'no-nans-float-math' flag) a combination of a fcmp and select has 'minimum'
-/// semantics. In the presence of 'NaN' we have to preserve the original
-/// select(fcmp(ult/le, L, R), L, R) semantics matched by this predicate.
-///
-///                          max(L, R)  iff L and R are not NaN
-///  m_UnordFMin(L, R) =     L          iff L or R are NaN
-template <typename LHS, typename RHS>
-inline MaxMin_match<FCmpInst, LHS, RHS, ufmin_pred_ty>
-m_UnordFMin(const LHS &L, const RHS &R) {
-  return MaxMin_match<FCmpInst, LHS, RHS, ufmin_pred_ty>(L, R);
-}
-
 template <typename Opnd_t> struct Argument_match {
   unsigned OpI;
   Opnd_t Val;
 
   Argument_match(unsigned OpIdx, const Opnd_t &V) : OpI(OpIdx), Val(V) {}
 
   template <typename OpTy> bool match(OpTy *V) {
     CallSite CS(V);
     return CS.isCall() && Val.match(CS.getArgument(OpI));

llvm/unittests/IR/PatternMatch.cpp

TEST_F(PatternMatchTest, FloatingPointOrderedMin) {
   // Test no match on OGE.
   EXPECT_FALSE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpOGE(L, R), L, R)));
 
   // Test no match on OGT.
   EXPECT_FALSE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpOGT(L, R), L, R)));
 
-  // Test match on OGE with inverted select.
-  EXPECT_TRUE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
+  // Test inverted selects. Note, that this "inverts" the ordering, e.g.:
+  // %cmp = fcmp oge L, R
+  // %min = select %cmp R, L
+  // Given L == NaN
+  // the above is expanded to %cmp == false ==> %min = L
+  // which is true for UnordFMin, not OrdFMin, so test that:
+
+  // [OU]GE with inverted select.
+  EXPECT_FALSE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
                   .match(IRB.CreateSelect(IRB.CreateFCmpOGE(L, R), R, L)));
+  EXPECT_TRUE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpUGE(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 
-  // Test match on OGT with inverted select.
-  EXPECT_TRUE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
+  // [OU]GT with inverted select.
+  EXPECT_FALSE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
                   .match(IRB.CreateSelect(IRB.CreateFCmpOGT(L, R), R, L)));
+  EXPECT_TRUE(m_OrdFMin(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpUGT(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 }
 
 TEST_F(PatternMatchTest, FloatingPointOrderedMax) {
   Type *FltTy = IRB.getFloatTy();
   Value *L = ConstantFP::get(FltTy, 1.0);
   Value *R = ConstantFP::get(FltTy, 2.0);
TEST_F(PatternMatchTest, FloatingPointOrderedMax) {
   // Test no match on OLE.
   EXPECT_FALSE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpOLE(L, R), L, R)));
 
   // Test no match on OLT.
   EXPECT_FALSE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpOLT(L, R), L, R)));
 
-  // Test match on OLE with inverted select.
-  EXPECT_TRUE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
+
+  // Test inverted selects. Note, that this "inverts" the ordering, e.g.:
+  // %cmp = fcmp ole L, R
+  // %max = select %cmp, R, L
+  // Given L == NaN,
+  // the above is expanded to %cmp == false ==> %max == L
+  // which is true for UnordFMax, not OrdFMax, so test that:
+
+  // [OU]LE with inverted select.
+  EXPECT_FALSE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpOLE(L, R), R, L)));
+  EXPECT_TRUE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpULE(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 
-  // Test match on OLT with inverted select.
-  EXPECT_TRUE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
+  // [OUT]LT with inverted select.
+  EXPECT_FALSE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpOLT(L, R), R, L)));
+  EXPECT_TRUE(m_OrdFMax(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpULT(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 }
 
 TEST_F(PatternMatchTest, FloatingPointUnorderedMin) {
   Type *FltTy = IRB.getFloatTy();
   Value *L = ConstantFP::get(FltTy, 1.0);
   Value *R = ConstantFP::get(FltTy, 2.0);
TEST_F(PatternMatchTest, FloatingPointUnorderedMin) {
   // Test no match on UGE.
   EXPECT_FALSE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpUGE(L, R), L, R)));
 
   // Test no match on UGT.
   EXPECT_FALSE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpUGT(L, R), L, R)));
 
-  // Test match on UGE with inverted select.
-  EXPECT_TRUE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
+  // Test inverted selects. Note, that this "inverts" the ordering, e.g.:
+  // %cmp = fcmp uge L, R
+  // %min = select %cmp R, L
+  // Given L == NaN
+  // the above is expanded to %cmp == true ==> %min = R
+  // which is true for OrdFMin, not UnordFMin, so test that:
+
+  // [UO]GE with inverted select.
+  EXPECT_FALSE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
                   .match(IRB.CreateSelect(IRB.CreateFCmpUGE(L, R), R, L)));
+  EXPECT_TRUE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpOGE(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 
-  // Test match on UGT with inverted select.
-  EXPECT_TRUE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
+  // [UO]GT with inverted select.
+  EXPECT_FALSE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
                   .match(IRB.CreateSelect(IRB.CreateFCmpUGT(L, R), R, L)));
+  EXPECT_TRUE(m_UnordFMin(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpOGT(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 }
 
 TEST_F(PatternMatchTest, FloatingPointUnorderedMax) {
   Type *FltTy = IRB.getFloatTy();
   Value *L = ConstantFP::get(FltTy, 1.0);
   Value *R = ConstantFP::get(FltTy, 2.0);
TEST_F(PatternMatchTest, FloatingPointUnorderedMax) {
   // Test no match on ULE.
   EXPECT_FALSE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpULE(L, R), L, R)));
 
   // Test no match on ULT.
   EXPECT_FALSE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
                    .match(IRB.CreateSelect(IRB.CreateFCmpULT(L, R), L, R)));
 
-  // Test match on ULE with inverted select.
-  EXPECT_TRUE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
+  // Test inverted selects. Note, that this "inverts" the ordering, e.g.:
+  // %cmp = fcmp ule L, R
+  // %max = select %cmp R, L
+  // Given L == NaN
+  // the above is expanded to %cmp == true ==> %max = R
+  // which is true for OrdFMax, not UnordFMax, so test that:
+
+  // [UO]LE with inverted select.
+  EXPECT_FALSE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
                   .match(IRB.CreateSelect(IRB.CreateFCmpULE(L, R), R, L)));
+  EXPECT_TRUE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpOLE(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 
-  // Test match on ULT with inverted select.
-  EXPECT_TRUE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
+  // [UO]LT with inverted select.
+  EXPECT_FALSE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
                   .match(IRB.CreateSelect(IRB.CreateFCmpULT(L, R), R, L)));
+  EXPECT_TRUE(m_UnordFMax(m_Value(MatchL), m_Value(MatchR))
+                  .match(IRB.CreateSelect(IRB.CreateFCmpOLT(L, R), R, L)));
   EXPECT_EQ(L, MatchL);
   EXPECT_EQ(R, MatchR);
 }
 
 TEST_F(PatternMatchTest, OverflowingBinOps) {
   Value *L = IRB.getInt32(1);
   Value *R = IRB.getInt32(2);
   Value *MatchL, *MatchR;