tl;dr ⚙ D140856 [mlir][Arith] Remove expansions of min and max ops makes -arith-expand-ops no longer expand min and max, please object if you still need that. I put most of my reasoning in the commit message on the revision, but, to summarize, both ArithToLLVM and ArithToSPIRV have their own conversion patterns for the various arith.min* and arith.max* operations, which are actually different between the two converters (LLVM doesn’t try to implement any NaN semantics, SPIRV does). So, since all of the conversions can handle the arith.{min,max}* operations, we don’t need tho expansion code anymore. All it’s doing right now is providing interesting footgun opportunities, IMO. Am I missing something? Is anyone relying on that code? (Tangentially, having gone and looked, it appears that our lowering to LLVM doesn’t preserve the NaN semastics we specified in the docs, since LLVM’s maximum follows C and says max(nan, X) = max(X, nan) = X. But since that’s an existing bug, making a decision on that is future work, I’d think.)

Did I misread the LLVM documenation and get the NaN behavior backwards, or does the IEEE operation have the opposite semantics to what LLVM implements?

Also, it looks like someone else noticed and put ⚙ D137786 Lower arith.min/max to llvm.intr.minimum/maximum together to move to the min/max propagate NaN semantics, which I assume is IEEE

You said said the right thing for the wrong intrinsic. There are at least 5 different versions of this operation: libm fmin/fmax: ignores signaling nans, returns non-nan. Corresponds to llvm.minnum/maxnum IEEE-754-2008 minnum/maxnum: signaling nan->qnan, returns non-nan, unspecified result for -0,+0 IEEE-754-2019 minnum/maxnum: same, except with specified -0 < +0 IEEE-754-2019 minimum/maximum: Corresponds to llvm.minimum/maximum, Propagates the nan operand rather than returns the non-nan operand Basic compare and select

It’s unbelievable how often this comes up. I think we need to rename minnum/maxnum and add the full set of intrinsics to fully disambiguate this.

Thanks for bringing this up! I think part of the problem is that when we introduced the arith.minf and arith.maxf ops it wasn’t clear what behavior we wanted to model in MLIR regarding NaN, ±0.0 and friends. I think we just propagated the documentation from LLVM. In addition to the NaN misalignment, for example, I don’t think we ever honored the treating -0.0 as less than +0.0 behavior. IMO, the first step to disentangle this would be to make a call on the variants we want to represent in MLIR, which is not easy at all. [image] arsenm: libm fmin/fmax: ignores signaling nans, returns non-nan. Corresponds to llvm.minnum/maxnum IEEE-754-2008 minnum/maxnum: signaling nan->qnan, returns non-nan, unspecified result for -0,+0 IEEE-754-2019 minnum/maxnum: same, except with specified -0 < +0 IEEE-754-2019 minimum/maximum: Corresponds to llvm.minimum/maximum, Propagates the nan operand rather than returns the non-nan operand Basic compare and select In addition to the variants above we should also consider the variants from the ML frameworks, which may or may not fall into any of the above. IIRC, there were some subtleties and description gaps in some of the framework specs about the handling of the special floating-point numbers that make this problem even more challenging. We should also take into account that whatever we decide about standalone fp min and max ops should be propagated to the reduction ops in some MLIR dialects (e.g., vector). Perhaps aligning MLIR with the variants represented in LLVM would be a good way to go. I’m not familiar with the SPIRV ones. Are they significantly different?

[image] dcaballe: Perhaps aligning MLIR with the variants represented in LLVM would be a good way to go. I’m not familiar with the SPIRV ones. Are they significantly different? The opencl ones match C99 (signaling NaNs may behave as quiet NaNs), the opengl variants are similar but less specific on NaNs. -0/+0 handling is not specified either. So we may not be able to lower better defined min/max ops to SPIR-V.

We currently lower the min and max ops to spir-v by adding the NaN checks ourselves … except when -arith-expand-ops is used, which will cause that lowering to be skipped.

We can check for NaN and Inf but I’m not sure if there’s something efficient we could lower to to guarantee -0/+0 ordering.

(Thinking out loud) I’m wondering if adding attributes to arith.minf and arith.maxf to describe the NaN and 0.0 handling would be reasonable in this case vs having independent ops for each variant. I know this approach is usually discouraged but I have the impression that the NaN/Inf/0.0 handling is a property that is not so common to be reasoned about during high-level transformations/optimizations (with obvious exceptions, of course) and it’s more a property used at lowering time. If that is the case, I see more value in representing the core fp min/max semantics with single ops than having too many variants of them precisely modeling semantic aspects that perhaps are nots so valuable at this level of abstraction. To some extent, this is not so different from the ordered/unordered behavior in fp compare operations… I think having the right coverage here is important because the NaN/Inf/0.0 handling can be more expensive than the min/max operation itself and it wouldn’t make sense to enforce a specific handling for specs that do not require them.

Yeah. I’d be willing to temporarily declare “the NaN and ± 0 semantics are platform-dependent, please check if you care” for arith.{min,max}f.

[Proposed breaking change/RFC] Remove min and max from -arith-expand-ops

MLIR

krzysz00 January 6, 2023, 8:31pm 21

Well, here’s the RFC [RFC] Explicitly specifying NaN and signed zero semantics for minf and maxf

Topic		Replies	Views
[RFC] Fix floating-point `max` and `min` operations in MLIR MLIR rfc	8	1745	August 21, 2023
[RFC] Explicitly specifying NaN and signed zero semantics for minf and maxf MLIR	24	1673	January 11, 2023
RFC: Add IEEE 754-2019 minimumNumber/maximumNumber operations to Arith MLIR	18	580	October 5, 2026
Adding min/max (int/float) ops to standard dialect MLIR	6	1323	August 11, 2021
[PATCH][RFC]: Add fmin/fmax intrinsics LLVM Dev List Archives	23	729	October 9, 2014

[Proposed breaking change/RFC] Remove min and max from -arith-expand-ops

Related topics