Instcombile min/max intrinsics calls

[karthikthecool]

Summary

Adding a small patch to fold the following min/max intrinsics calls-

1. fmin(x, fmax(x, y)) -> select isnan(x)?y:x
2. fmax(x, fmin(x, y)) -> select isnan(x)?y:x

Thanks

[escha]

Counterexample:

x = NaN

y = 0

fmin(x, fmax(x, y)) -> x ?
fmin(x, fmax(NaN, 0)) -> x ?
fmin(NaN, 0) -> x ?
fmin(NaN, 0) -> 0
0 != x

[karthikthecool]

Thanks escha for the counter example. I will handle NaN case properly.
Apart from that it should work properly right?

[karthikthecool]

Update the patch to handle NaN's.
As escha suggested the previous code will fail in case on NaN.
We will need runtime checks to see if the value is NaN and select X or Y accordingly.

We eliminate 2 intinsic function call with a compare and select instruction.
Please let me know your valuable inputs on the same and if it requires any modifications.

Thanks and Regards
Karthik Bhat

[arsenm]

Should have some tests with known non-nan cases

[majnemer]

I believe you can also check for nnan on the fmin/fmax to further improve the IR.

[karthikthecool]

Hi David, Matt,
Thanks a lot for the review comments. Please find my comments below-

@Matt: I have already added test cases for non nan cases which are ( maxnum_x_y_minnum_y, innum_x_maxnum_x_y. minnum_x_y_maxnum_y, maxnum_x_minnum_x_y) respectively. I feel these should cover all the 4 cases for which patch was added. Please let me know if more test cases are required.

@david: fmin/fmax already handle nan when we can deduce a parameter as "nan at compile time".
If we were to add check at runtime the code will look something like -

fmin(x,y) -> if(isnan(x|y)) ? ( if(isnan(x)) ? y : x) : fmin(x,y)

for non nan cases which i believe will be mostly called we will have one extra compare and select instruction in addition to the normal fmin call.
I feel it may not be worth adding this runtime check. But please let me know if you feel otherwise. I can update the code accordingly.

Thanks again for your review comments. Please let me know your valuable inputs on the comments above.

Regards
Karthik

[majnemer]

In D21137#454408, @karthikthecool wrote:

Hi David, Matt,
Thanks a lot for the review comments. Please find my comments below-

@Matt: I have already added test cases for non nan cases which are ( maxnum_x_y_minnum_y, innum_x_maxnum_x_y. minnum_x_y_maxnum_y, maxnum_x_minnum_x_y) respectively. I feel these should cover all the 4 cases for which patch was added. Please let me know if more test cases are required.

@david: fmin/fmax already handle nan when we can deduce a parameter as "nan at compile time".
If we were to add check at runtime the code will look something like -

fmin(x,y) -> if(isnan(x|y)) ? ( if(isnan(x)) ? y : x) : fmin(x,y)

for non nan cases which i believe will be mostly called we will have one extra compare and select instruction in addition to the normal fmin call.
I feel it may not be worth adding this runtime check. But please let me know if you feel otherwise. I can update the code accordingly.

I am saying that there are situations where we know a-priori that the fmax/fmin call will observe a nan due to annotations on the call that _cannot_ be deduced through other data flow analysis. I would just check that the call is marked nnan and choose not to emit a select in those situations.

Thanks again for your review comments. Please let me know your valuable inputs on the comments above.

Regards
Karthik

[karthikthecool]

Thanks David for the comments. I get your point now. Updated the code to incorporate comment.

Please let me know if you have any other feedback.
Thanks and Regards
Karthik Bhat

[karthikthecool]

Ping..

Thanks
Karthik

[majnemer]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1013–1014

Couldn't this case live in InstructionSimplify?

[karthikthecool]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1013–1014

Hi David,
Thanks for the comments.
Since in simplifyMinnumMaxnum we use
replaceInstUsesWith i thought we can reuse the same code here..
Also i feel handling fmin/fmax cases here in this single function would keep changes limited to one place.
But please let me knw if you feel this place is incorrect I will try to move these into instsimplify.
Thanks again for your time and review comments.
Regards
Karthik

[karthikthecool]

Ping..
Updated diff.

[arsenm]

I discovered that the libcall already does this nnan compare + select canonicalization (although I don't get the rationale for it, the opposite seems preferable). Are you still working on this patch?

lib/Transforms/InstCombine/InstCombineCalls.cpp

if (auto *CInt = dyn_cast<ConstantInt>(II.getArgOperand(2))) {
 
     if (Value *Cmp = Builder.CreateICmp(Pred, II.getArgOperand(0),
                                         II.getArgOperand(1)))
       return Builder.CreateSExtOrTrunc(Cmp, VecTy);
   }
   return nullptr;
 }
 
+Instruction *InstCombiner::getReducedInstruction(IntrinsicInst *II) {
+  Value *Arg0 = II->getArgOperand(0);
+  Value *Arg1 = II->getArgOperand(1);
+  Value *X = nullptr;
+  Value *Y = nullptr;
+
+  if (II->getIntrinsicID() == Intrinsic::maxnum) {
+    // fmax(x, fmin(x, y)) -> select isnan(x)?y:x
+    // fmax(y, fmin(x, y)) -> select isnan(y)?x:y
+    if (match(Arg1, m_FMin(m_Value(X), m_Value(Y)))) {
+      if (Arg0 == X || Arg0 == Y) {
+        IntrinsicInst *I = cast<IntrinsicInst>(Arg1);
+        if (I->hasNoNaNs())
+          return replaceInstUsesWith(*II, Arg0);
+        Instruction *Result = new FCmpInst(II, FCmpInst::FCMP_UNO, Arg0, Arg0);
+        return SelectInst::Create(Result, Arg0 == X ? Y : X, Arg0);
+      }
+    }
+    // fmax(fmin(x, y), x) -> select isnan(x)?y:x
+    // fmax(fmin(x, y), y) -> select isnan(y)?x:y
+    if (match(Arg0, m_FMin(m_Value(X), m_Value(Y)))) {
+      if (Arg1 == X || Arg1 == Y) {
+        IntrinsicInst *I = cast<IntrinsicInst>(Arg0);
+        if (I->hasNoNaNs())
+          return replaceInstUsesWith(*II, Arg1);
+        Instruction *Result = new FCmpInst(II, FCmpInst::FCMP_UNO, Arg1, Arg1);
+        return SelectInst::Create(Result, Arg1 == X ? Y : X, Arg1);
+      }
+    }
+
+  } else {
+    // fmin(x, fmax(x, y)) -> select isnan(x)?y:x
+    // fmin(y, fmax(x, y)) -> select isnan(y)?x:y
+    if (match(Arg1, m_FMax(m_Value(X), m_Value(Y)))) {
+      if (Arg0 == X || Arg0 == Y) {
+        IntrinsicInst *I = cast<IntrinsicInst>(Arg1);
+        if (I->hasNoNaNs())
+          return replaceInstUsesWith(*II, Arg0);
+        Instruction *Result = new FCmpInst(II, FCmpInst::FCMP_UNO, Arg0, Arg0);
+        return SelectInst::Create(Result, Arg0 == X ? Y : X, Arg0);
+      }
+    }
+
+    // fmin(fmax(x, y), x) -> select isnan(x)?y:x
+    // fmin(fmax(x, y), y) -> select isnan(y)?x:y
+    if (match(Arg0, m_FMax(m_Value(X), m_Value(Y)))) {
+      if (Arg1 == X || Arg1 == Y) {
+        IntrinsicInst *I = cast<IntrinsicInst>(Arg0);
+        if (I->hasNoNaNs())
+          return replaceInstUsesWith(*II, Arg1);
+        Instruction *Result = new FCmpInst(II, FCmpInst::FCMP_UNO, Arg1, Arg1);
+        return SelectInst::Create(Result, Arg1 == X ? Y : X, Arg1);
+      }
+    }
+  }
+
+  return nullptr;
+}
+
 static Value *simplifyMinnumMaxnum(const IntrinsicInst &II) {
   Value *Arg0 = II.getArgOperand(0);
   Value *Arg1 = II.getArgOperand(1);
 
   // fmin(x, x) -> x
   if (Arg0 == Arg1)
     return Arg0;
 
case Intrinsic::maxnum: {
     // Canonicalize constants to the RHS.
     if (isa<ConstantFP>(Arg0) && !isa<ConstantFP>(Arg1)) {
       II->setArgOperand(0, Arg1);
       II->setArgOperand(1, Arg0);
       return II;
     }
     if (Value *V = simplifyMinnumMaxnum(*II))
       return replaceInstUsesWith(*II, V);
+
+    if (Instruction *I = getReducedInstruction(II))
+      return I;
+
     break;
   }
   case Intrinsic::ppc_altivec_lvx:
   case Intrinsic::ppc_altivec_lvxl:
     // Turn PPC lvx -> load if the pointer is known aligned.
     if (getOrEnforceKnownAlignment(II->getArgOperand(0), 16, DL, II, &AC,
                                    &DT) >= 16) {
       Value *Ptr = Builder->CreateBitCast(II->getArgOperand(0),

lib/Transforms/InstCombine/InstCombineInternal.h

private:
   const DataLayout &DL;
 
   // Optional analyses. When non-null, these can both be used to do better
   // combining and will be updated to reflect any changes.
   LoopInfo *LI;
 
   bool MadeIRChange;
 
+  Instruction *getReducedInstruction(IntrinsicInst *II);
+
 public:
   InstCombiner(InstCombineWorklist &Worklist, BuilderTy *Builder,
                bool MinimizeSize, bool ExpensiveCombines, AliasAnalysis *AA,
                AssumptionCache &AC, TargetLibraryInfo &TLI,
                DominatorTree &DT, const DataLayout &DL, LoopInfo *LI)
       : Worklist(Worklist), Builder(Builder), MinimizeSize(MinimizeSize),
         ExpensiveCombines(ExpensiveCombines), AA(AA), AC(AC), TLI(TLI), DT(DT),
         DL(DL), LI(LI), MadeIRChange(false) {}

test/Transforms/InstCombine/maxnum.ll

 ; RUN: opt -S -instcombine < %s | FileCheck %s
 
 declare float @llvm.maxnum.f32(float, float) #0
+declare float @llvm.minnum.f32(float, float) #0
 declare float @llvm.maxnum.v2f32(<2 x float>, <2 x float>) #0
 declare <4 x float> @llvm.maxnum.v4f32(<4 x float>, <4 x float>) #0
 
 declare double @llvm.maxnum.f64(double, double) #0
 declare <2 x double> @llvm.maxnum.v2f64(<2 x double>, <2 x double>) #0
 
 ; CHECK-LABEL: @constant_fold_maxnum_f32
 ; CHECK-NEXT: ret float 2.000000e+00

 ; CHECK-LABEL: @fold_maxnum_f32_neginf_val
 ; CHECK-NEXT: call float @llvm.maxnum.f32(float %x, float 0xFFF0000000000000)
 ; CHECK-NEXT: ret float
 define float @fold_maxnum_f32_neginf_val(float %x) nounwind {
   %val = call float @llvm.maxnum.f32(float 0xFFF0000000000000, float %x) #0
   ret float %val
 }
 
+; CHECK-LABEL: @maxnum_x_minnum_x_y
+; CHECK-NEXT: %1 = fcmp uno float %x, 0.000000e+00
+; CHECK-NEXT: %b = select i1 %1, float %y, float %x
+; CHECK-NEXT: ret float %b
+define float @maxnum_x_minnum_x_y(float %x, float %y) #0 {
+  %a = call float @llvm.minnum.f32(float %x, float %y) #0
+  %b = call float @llvm.maxnum.f32(float %x, float %a) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @maxnum_x_minnum_x_y_nnan
+; CHECK-NEXT: ret float %x
+define float @maxnum_x_minnum_x_y_nnan(float %x, float %y) #0 {
+  %a = call nnan float @llvm.minnum.f32(float %x, float %y) #0
+  %b = call float @llvm.maxnum.f32(float %x, float %a) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @minnum_x_y_maxnum_y
+; CHECK-NEXT: %1 = fcmp uno float %y, 0.000000e+00
+; CHECK-NEXT: %b = select i1 %1, float %x, float %y
+; CHECK-NEXT: ret float %b
+define float @minnum_x_y_maxnum_y(float %x, float %y) #0 {
+  %a = call float @llvm.minnum.f32(float %x, float %y) #0
+  %b = call float @llvm.maxnum.f32(float %a, float %y) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @minnum_x_y_maxnum_y_nnan
+; CHECK-NEXT: ret float %y
+define float @minnum_x_y_maxnum_y_nnan(float %x, float %y) #0 {
+  %a = call nnan float @llvm.minnum.f32(float %x, float %y) #0
+  %b = call float @llvm.maxnum.f32(float %a, float %y) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @minnum_x_nan_maxnum_nan
+; CHECK-NEXT: ret float %x
+define float @minnum_x_nan_maxnum_nan(float %x) #0 {
+  %a = call float @llvm.minnum.f32(float %x, float 0x7FF8000000000000) #0
+  %b = call float @llvm.maxnum.f32(float %a, float 0x7FF8000000000000) #0
+  ret float %b
+}
+
 attributes #0 = { nounwind readnone }

test/Transforms/InstCombine/minnum.ll

 ; RUN: opt -S -instcombine < %s | FileCheck %s
 
 declare float @llvm.minnum.f32(float, float) #0
+declare float @llvm.maxnum.f32(float, float) #0
 declare float @llvm.minnum.v2f32(<2 x float>, <2 x float>) #0
 declare <4 x float> @llvm.minnum.v4f32(<4 x float>, <4 x float>) #0
 
 declare double @llvm.minnum.f64(double, double) #0
 declare <2 x double> @llvm.minnum.v2f64(<2 x double>, <2 x double>) #0
 
-declare float @llvm.maxnum.f32(float, float) #0
 
 ; CHECK-LABEL: @constant_fold_minnum_f32
 ; CHECK-NEXT: ret float 1.000000e+00
 define float @constant_fold_minnum_f32() #0 {
   %x = call float @llvm.minnum.f32(float 1.0, float 2.0) #0
   ret float %x
 }
 

 ; CHECK-NEXT: ret float
 define float @minnum4(float %x, float %y, float %z, float %w) #0 {
   %a = call float @llvm.minnum.f32(float %x, float %y) #0
   %b = call float @llvm.minnum.f32(float %z, float %w) #0
   %c = call float @llvm.minnum.f32(float %a, float %b) #0
   ret float %c
 }
 
-; CHECK-LABEL: @minnum_x_maxnum_x_y
-; CHECK-NEXT: call float @llvm.maxnum.f32
-; CHECK-NEXT: call float @llvm.minnum.f32
-; CHECK-NEXT: ret float
-define float @minnum_x_maxnum_x_y(float %x, float %y) #0 {
-  %a = call float @llvm.maxnum.f32(float %x, float %y) #0
-  %b = call float @llvm.minnum.f32(float %x, float %a) #0
-  ret float %b
-}
-
-; CHECK-LABEL: @maxnum_x_minnum_x_y
-; CHECK-NEXT: call float @llvm.minnum.f32
-; CHECK-NEXT: call float @llvm.maxnum.f32
-; CHECK-NEXT: ret float
-define float @maxnum_x_minnum_x_y(float %x, float %y) #0 {
-  %a = call float @llvm.minnum.f32(float %x, float %y) #0
-  %b = call float @llvm.maxnum.f32(float %x, float %a) #0
-  ret float %b
-}
-
 ; CHECK-LABEL: @fold_minnum_f32_inf_val
 ; CHECK-NEXT: call float @llvm.minnum.f32(float %x, float 0x7FF0000000000000)
 ; CHECK-NEXT: ret float
 define float @fold_minnum_f32_inf_val(float %x) nounwind {
   %val = call float @llvm.minnum.f32(float 0x7FF0000000000000, float %x) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_minnum_f32_minf_val
 ; CHECK-NEXT: ret float 0xFFF0000000000000
 define float @fold_minnum_f32_minf_val(float %x) nounwind {
   %val = call float @llvm.minnum.f32(float 0xFFF0000000000000, float %x) #0
   ret float %val
 }
 
+; CHECK-LABEL: @minnum_x_maxnum_x_y
+; CHECK-NEXT: %1 = fcmp uno float %x, 0.000000e+00
+; CHECK-NEXT: %b = select i1 %1, float %y, float %x
+; CHECK-NEXT: ret float %b
+
+define float @minnum_x_maxnum_x_y(float %x, float %y) #0 {
+  %a = call float @llvm.maxnum.f32(float %x, float %y) #0
+  %b = call float @llvm.minnum.f32(float %x, float %a) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @minnum_x_maxnum_x_y_nnan
+; CHECK-NEXT: ret float %x
+
+define float @minnum_x_maxnum_x_y_nnan(float %x, float %y) #0 {
+  %a = call nnan float @llvm.maxnum.f32(float %x, float %y) #0
+  %b = call float @llvm.minnum.f32(float %x, float %a) #0
+  ret float %b
+}
+
+
+; CHECK-LABEL: @maxnum_x_y_minnum_y
+; CHECK-NEXT: %1 = fcmp uno float %y, 0.000000e+00
+; CHECK-NEXT: %b = select i1 %1, float %x, float %y
+; CHECK-NEXT: ret float %b
+define float @maxnum_x_y_minnum_y(float %x, float %y) #0 {
+  %a = call float @llvm.maxnum.f32(float %x, float %y) #0
+  %b = call float @llvm.minnum.f32(float %a, float %y) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @maxnum_x_y_minnum_y_nnan
+; CHECK-NEXT: ret float %y
+define float @maxnum_x_y_minnum_y_nnan(float %x, float %y) #0 {
+  %a = call nnan float @llvm.maxnum.f32(float %x, float %y) #0
+  %b = call float @llvm.minnum.f32(float %a, float %y) #0
+  ret float %b
+}
+
+; CHECK-LABEL: @maxnum_y_nan_minnum_nan
+; CHECK-NEXT: ret float %y
+define float @maxnum_y_nan_minnum_nan(float %y) #0 {
+  %a = call float @llvm.maxnum.f32(float 0x7FF8000000000000, float %y) #0
+  %b = call float @llvm.minnum.f32(float %a,  float 0x7FF8000000000000) #0
+  ret float %b
+}
+
 attributes #0 = { nounwind readnone }