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

MLIR
1

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.)

1 Like
2

since LLVM’s maximum follows C and says max(nan, X) = max(X, nan) = X.

That’s minnum/maxnum (which really, really should be named fmin/fmax). maximum is the new IEEE-754 2019 operation that has the opposite nan behavior

3

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?

4

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

5

You said said the right thing for the wrong intrinsic.

There are at least 5 different versions of this operation:

  1. libm fmin/fmax: ignores signaling nans, returns non-nan. Corresponds to llvm.minnum/maxnum
  2. IEEE-754-2008 minnum/maxnum: signaling nan->qnan, returns non-nan, unspecified result for -0,+0
  3. IEEE-754-2019 minnum/maxnum: same, except with specified -0 < +0
  4. IEEE-754-2019 minimum/maximum: Corresponds to llvm.minimum/maximum, Propagates the nan operand rather than returns the non-nan operand
  5. Basic compare and select
6

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.

7

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.

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?

8

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.

9

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.

10

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.

11

(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.

12

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.

13

I think the right answer is just have all the operations and pick which ones you want. They’re distinct operations, part of the problem is things trying to stuff different operations in the same name.

14

Would folks be OK with arith.maxf and arith.minf becoming changed from “nan inputs produces nan” to “what happens to nan inputs depends on the lowering”?

15

I think this wouldn’t work. The semantics of an operation shouldn’t be defined based on its lowering. They should be properly defined in the spec of the operation.

I’m ok with introducing a district operation for each variant if people feel that’s the best way to go. My only concern with this approach is that we may end up with a combinatorial number of variants that are all treated as a single op in 99% of the code at this level of abstraction. We could also introduce a MaxFOpInterface and MinFOpInterface, which would be implemented by each variant, for those cases where we want to treat them all as a single op, but again, if these interfaces are used 99% of the time I’m not sure about the value of having distinct operations for each variant.

Let’s see if more people have an opinion about this: @mehdi_amini, @jpienaar, @River707, @ftynse, @clattner

2 Likes
16

… maybe slightly better idea. arith.minf and arith.maxf take NaNSemantics and SignedZeroSemantics attributes.

I’d suggest NanSemantics = Unspecified, PropagateNan, PropagateOther and SignedZeroSemantics = Undefined, Compares

17

For the short term, though. I’ll take my expansion removal patch and get the minf/maxf bits out of it, so we just remove expanding integer max/min

18

Using attributes seems reasonable to me here, if the attributes don’t change fundamental properties of the operation (does it stay commutative/associative? etc.)

19

Agreed. Ok, so this probably needs to be a new RFC. … but while we’re here, are there any other float operations other than min and max that have all these variants?

20

Potentially any reduction operation that can have an fp min/max combiner (e.g., vector.reduction, vector.multi_reduction). Not sure if we have more in other dialects.