Lower arith.min/max to llvm.intr.minimum/maximum

[gflegar]

Summary

The current lowering was converting it into llvm.intr.minnum/maxnum which has
incompattible NaN behavior to the one specified by the arit.min/max ops.

Depends On D137655

[kuhar]

Could you update the patch description with a brief description of the new/intended behavior? This way it would be easier to tell what the change entails without cross-checking with the llvm language reference.

incompattible NaN behavior to the one specified by the arit.min/max ops.

nit: typo in 'arith'

[krzysz00]

Should someone (maybe me) go ahead and land this?

[Mogball]

This patch is depending on another one

[krzysz00]

I'd like to request comment on https://discourse.llvm.org/t/rfc-explicitly-specifying-nan-and-signed-zero-semantics-for-minf-and-maxf/67539 , as it relates to the topic of this patch

[arsenm]

I don't think we even have a legalization implementation to expand these. You almost certainly intended to use llvm.minnum/maxnum

[gflegar]

In D137786#4032742, @arsenm wrote:

I don't think we even have a legalization implementation to expand these. You almost certainly intended to use llvm.minnum/maxnum

No, I did want to use llvm.minimum/maximum, because those are the instructions that have the correct semantics for arith.min/max.
We can't expand those in LLVM now, but that's what the dependent patch is for, however it needs more work to correctly expand it in a way that handles +/-0.0.
Don't have the cycles to figure that out at the moment though, so we can put this patch on hold for now (unless someone else wants to pick it up).

This should also answer @krzysz00's question: we can't land before the dependent patch is figured out.

mlir/lib/Conversion/ArithToLLVM/ArithToLLVM.cpp

using ExtSIOpLowering =
     VectorConvertToLLVMPattern<arith::ExtSIOp, LLVM::SExtOp>;
 using ExtUIOpLowering =
     VectorConvertToLLVMPattern<arith::ExtUIOp, LLVM::ZExtOp>;
 using FPToSIOpLowering =
     VectorConvertToLLVMPattern<arith::FPToSIOp, LLVM::FPToSIOp>;
 using FPToUIOpLowering =
     VectorConvertToLLVMPattern<arith::FPToUIOp, LLVM::FPToUIOp>;
 using MaxFOpLowering =
-    VectorConvertToLLVMPattern<arith::MaxFOp, LLVM::MaxNumOp,
+    VectorConvertToLLVMPattern<arith::MaxFOp, LLVM::MaximumOp,
                                arith::AttrConvertFastMathToLLVM>;
 using MaxSIOpLowering =
     VectorConvertToLLVMPattern<arith::MaxSIOp, LLVM::SMaxOp>;
 using MaxUIOpLowering =
     VectorConvertToLLVMPattern<arith::MaxUIOp, LLVM::UMaxOp>;
 using MinFOpLowering =
-    VectorConvertToLLVMPattern<arith::MinFOp, LLVM::MinNumOp,
+    VectorConvertToLLVMPattern<arith::MinFOp, LLVM::MinimumOp,
                                arith::AttrConvertFastMathToLLVM>;
 using MinSIOpLowering =
     VectorConvertToLLVMPattern<arith::MinSIOp, LLVM::SMinOp>;
 using MinUIOpLowering =
     VectorConvertToLLVMPattern<arith::MinUIOp, LLVM::UMinOp>;
 using MulFOpLowering =
     VectorConvertToLLVMPattern<arith::MulFOp, LLVM::FMulOp,
                                arith::AttrConvertFastMathToLLVM>;

mlir/test/Conversion/ArithToLLVM/arith-to-llvm.mlir

func.func @minmaxi(%arg0 : i32, %arg1 : i32) -> i32 {
   %2 = arith.minui %arg0, %arg1 : i32
   // CHECK: = llvm.intr.umax(%arg0, %arg1) : (i32, i32) -> i32
   %3 = arith.maxui %arg0, %arg1 : i32
   return %0 : i32
 }
 
 // CHECK-LABEL: @minmaxf
 func.func @minmaxf(%arg0 : f32, %arg1 : f32) -> f32 {
-  // CHECK: = llvm.intr.minnum(%arg0, %arg1) : (f32, f32) -> f32
+  // CHECK: = llvm.intr.minimum(%arg0, %arg1) : (f32, f32) -> f32
   %0 = arith.minf %arg0, %arg1 : f32
-  // CHECK: = llvm.intr.maxnum(%arg0, %arg1) : (f32, f32) -> f32
+  // CHECK: = llvm.intr.maximum(%arg0, %arg1) : (f32, f32) -> f32
   %1 = arith.maxf %arg0, %arg1 : f32
   return %0 : f32
 }
 
 // -----
 
 // CHECK-LABEL: @fastmath
 func.func @fastmath(%arg0: f32, %arg1: f32, %arg2: i32) {

 // -----
 
 // CHECK-LABEL: @ops_supporting_fastmath
 func.func @ops_supporting_fastmath(%arg0: f32, %arg1: f32, %arg2: i32) {
 // CHECK: llvm.fadd %arg0, %arg1  {fastmathFlags = #llvm.fastmath<fast>} : f32
   %0 = arith.addf %arg0, %arg1 fastmath<fast> : f32
 // CHECK: llvm.fdiv %arg0, %arg1  {fastmathFlags = #llvm.fastmath<fast>} : f32
   %1 = arith.divf %arg0, %arg1 fastmath<fast> : f32
-// CHECK: llvm.intr.maxnum(%arg0, %arg1) {fastmathFlags = #llvm.fastmath<fast>} : (f32, f32) -> f32
+// CHECK: llvm.intr.maximum(%arg0, %arg1) {fastmathFlags = #llvm.fastmath<fast>} : (f32, f32) -> f32
   %2 = arith.maxf %arg0, %arg1 fastmath<fast> : f32
-// CHECK: llvm.intr.minnum(%arg0, %arg1) {fastmathFlags = #llvm.fastmath<fast>} : (f32, f32) -> f32
+// CHECK: llvm.intr.minimum(%arg0, %arg1) {fastmathFlags = #llvm.fastmath<fast>} : (f32, f32) -> f32
   %3 = arith.minf %arg0, %arg1 fastmath<fast> : f32
 // CHECK: llvm.fmul %arg0, %arg1  {fastmathFlags = #llvm.fastmath<fast>} : f32
   %4 = arith.mulf %arg0, %arg1 fastmath<fast> : f32
 // CHECK: llvm.fneg %arg0  {fastmathFlags = #llvm.fastmath<fast>} : f32
   %5 = arith.negf %arg0 fastmath<fast> : f32
 // CHECK: llvm.frem %arg0, %arg1  {fastmathFlags = #llvm.fastmath<fast>} : f32
   %6 = arith.remf %arg0, %arg1 fastmath<fast> : f32
 // CHECK: llvm.fsub %arg0, %arg1  {fastmathFlags = #llvm.fastmath<fast>} : f32
   %7 = arith.subf %arg0, %arg1 fastmath<fast> : f32
   return
 }