Remove `Commutative` interface from `fmin/fmax`

[csigg]

Summary

Manually move constants to the right in the folder.

Kiran brought up in https://reviews.llvm.org/D117010 that fmin/fmax might not be commutative when NaNs are involved. That depends on which NaNs are considered 'same' (could be: IEEE, memcmp, all NaN are equal irrespective of the payload). Only the last one makes fmin/fmax commutative.

This change removes the Commutative op interface from fmin/fmax again to avoid the ambiguity, while still moving constants to the right hand side.

[rriddle]

Something I'm missing here is why it would be legal to swap operands if the operation isn't commutative.

[csigg]

In D118318#3274828, @rriddle wrote:

Something I'm missing here is why it would be legal to swap operands if the operation isn't commutative.

It's safe to swap operands because maxf/minf only specify that "If one of the arguments is NaN, then the result is also NaN.".

However, maxf/minf are not commutative in the IEEE sense if any operand is NaN, or in the memcmp sense for two NaN operands with different payload.

[mehdi_amini]

Can you elaborate a bit with an example of why the commutativity isn't there with NaN?

[csigg]

In D118318#3274867, @mehdi_amini wrote:

Can you elaborate a bit with an example of why the commutativity isn't there with NaN?

x = maxf(0x7fc00000, 0x7fffffff)
y = maxf(0x7fffffff, 0x7fc00000)

• not commutative in the IEEE sense: x != y
• not commutative in the memcmp sense: most likely memcmp(&x, &y, sizeof(x)) != 0
• commutative in the sense that both x and y are NaNs

[mehdi_amini]

In D118318#3275000, @csigg wrote:

In D118318#3274867, @mehdi_amini wrote:

Can you elaborate a bit with an example of why the commutativity isn't there with NaN?

x = maxf(0x7fc00000, 0x7fffffff)
y = maxf(0x7fffffff, 0x7fc00000)

• not commutative in the IEEE sense: x != y
• not commutative in the memcmp sense: most likely memcmp(&x, &y, sizeof(x)) != 0
• commutative in the sense that both x and y are NaNs

Thanks!
So it seems that you're assuming that one of the input value is returned when there is a NaN, and it would consistently be based on its position in the argument list. Is this how we define it?
Another definition could be that payload isn't guaranteed to be carried over and that we return a non-specific NaN that can be different from either of the argument.
Where is the minf definition coming from ("If one of the arguments is NaN, then the result is also NaN." is a bit underspecified), can we anchor ourselves to another spec?

[rriddle]

In D118318#3275064, @mehdi_amini wrote:

In D118318#3275000, @csigg wrote:

In D118318#3274867, @mehdi_amini wrote:

Can you elaborate a bit with an example of why the commutativity isn't there with NaN?

x = maxf(0x7fc00000, 0x7fffffff)
y = maxf(0x7fffffff, 0x7fc00000)

• not commutative in the IEEE sense: x != y
• not commutative in the memcmp sense: most likely memcmp(&x, &y, sizeof(x)) != 0
• commutative in the sense that both x and y are NaNs

Thanks!
So it seems that you're assuming that one of the input value is returned when there is a NaN, and it would consistently be based on its position in the argument list. Is this how we define it?
Another definition could be that payload isn't guaranteed to be carried over and that we return a non-specific NaN that can be different from either of the argument.
Where is the minf definition coming from ("If one of the arguments is NaN, then the result is also NaN." is a bit underspecified), can we anchor ourselves to another spec?

Yeah, it'd be nice to get a clear definition here. Moving constants to the right side feels really brittle for a non-commutative operation, given that any number of things could tweak the end result (e.g. "I added a new folder for an input operation and now my result is different").

[mehdi_amini]

Moving constants to the right side feels really brittle for a non-commutative operation

It isn't uncommon for some floating point operation though: the operation can have a commutative behavior other than if you know that the input isn't NaN, like when you have a constant :)

[mehdi_amini]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

695

These swaps are only valid when the constant isn't a NaN I think, otherwise you replicate the same issue you mentioned potentially?

[csigg]

So it seems that you're assuming that one of the input value is returned when there is a NaN, and it would consistently be based on its position in the argument list. Is this how we define it?
Another definition could be that payload isn't guaranteed to be carried over and that we return a non-specific NaN that can be different from either of the argument.

I tried not to make any assumptions, which is why I wrote "most likely". The point there is that the opposite (memcmp(&x, &y, sizeof(x)) == 0) is not guaranteed, no matter if we consistently return lhs or rhs, or if we returned a non-specific NaN.

Where is the minf definition coming from ("If one of the arguments is NaN, then the result is also NaN." is a bit underspecified), can we anchor ourselves to another spec?

The definition is from ArithmeticOps.td here.

We could adopt ARM's vmax or PTX's max.NaN, which returns a "default NaN".
This implementation of IEEE fminimum in SSE also returns a canonical NaN.

That would make minf/maxf commutative in the memcmp sense, but still not in the IEEE sense.

It might make the generated code slower for targets that have min/max instructions which don't return a canonical NaN though.

Yeah, it'd be nice to get a clear definition here.

The definition seems clear, it just doesn't specify the payload of the returned NaN.
I'm very much not an expert, but it seems better to not specify more than what a user would require ("it propagates NaNs") from the op, and leave more freedom to the lowering.
If some user really wants to carry over one of the operands' payload or wants to return a canonical NaN, he would need to use fcmp+select instead.

Some other data points:
IEEE 754-2019's minimum specifies to return "a quiet NaN if either operand is a NaN".
The llvm.minimum instruction specifies that it's NaN propagating.

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

695

According to the current specification, minf/maxf need return NaN if any argument is NaN and we are therefore free to swap operands.

[mehdi_amini]

The definition seems clear, it just doesn't specify the payload of the returned NaN.
I'm very much not an expert, but it seems better to not specify more than what a user would require ("it propagates NaNs") from the op, and leave more freedom to the lowering.
If some user really wants to carry over one of the operands' payload or wants to return a canonical NaN, he would need to use fcmp+select instead.

I understand all this as "keeping commutative is fine" isn't it?

[csigg]

I understand all this as "keeping commutative is fine" isn't it?

I'm having a hard time coming to that (or any, really) conclusion. I would welcome an 'executive decision'.

The commutative trait says

This trait adds the property that the operation is commutative, i.e. X op Y == Y op X.

without being specific what == means.
Removing the commutative trait from minf/maxf avoids this ambiguity.

If we do keep the commutative trait here, should we also add it to addf and mulf?

[mehdi_amini]

In D118318#3278476, @csigg wrote:

I understand all this as "keeping commutative is fine" isn't it?

I'm having a hard time coming to that (or any, really) conclusion. I would welcome an 'executive decision'.

The commutative trait says

This trait adds the property that the operation is commutative, i.e. X op Y == Y op X.

without being specific what == means.

I see what you mean about ==, my take on the definition is that "the compiler is allowed to swap the operands".
We should double check with others and clarify.

Removing the commutative trait from minf/maxf avoids this ambiguity.

If we do keep the commutative trait here, should we also add it to addf and mulf?

I don't know of a case that would make addf and mulf non commutative, you're thinking about similar issues as this patch?

[csigg]

We have settled on marking addf/mulf commutative as well (D118600) instead of removing it from minf/maxf. Abandoning this revision.

Thanks a lot Mehdi for all your help with this!

mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td

def Arith_SubFOp : Arith_FloatBinaryOp<"subf"> {
   let summary = "floating point subtraction operation";
   let hasFolder = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // MaxFOp
 //===----------------------------------------------------------------------===//
 
-def Arith_MaxFOp : Arith_FloatBinaryOp<"maxf", [Commutative]> {
+def Arith_MaxFOp : Arith_FloatBinaryOp<"maxf"> {
   let summary = "floating-point maximum operation";
   let description = [{
     Syntax:
 
     ```
     operation ::= ssa-id `=` `arith.maxf` ssa-use `,` ssa-use `:` type
     ```
 
def Arith_MaxUIOp : Arith_IntBinaryOp<"maxui"> {
   let summary = "unsigned integer maximum operation";
   let hasFolder = 1;
 }
 
 //===----------------------------------------------------------------------===//
 // MinFOp
 //===----------------------------------------------------------------------===//
 
-def Arith_MinFOp : Arith_FloatBinaryOp<"minf", [Commutative]> {
+def Arith_MinFOp : Arith_FloatBinaryOp<"minf"> {
   let summary = "floating-point minimum operation";
   let description = [{
     Syntax:
 
     ```
     operation ::= ssa-id `=` `arith.minf` ssa-use `,` ssa-use `:` type
     ```
 

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

 
 OpFoldResult arith::MaxFOp::fold(ArrayRef<Attribute> operands) {
   assert(operands.size() == 2 && "maxf takes two operands");
 
   // maxf(x,x) -> x
   if (getLhs() == getRhs())
     return getRhs();
 
+  // maxf(c,x) -> maxf(x,c)
+  if (operands.front() && !operands.back()) {
+    std::swap(getOperation()->getOpOperand(0), getOperation()->getOpOperand(1));
+    return getResult();
+  }
+
   return constFoldBinaryOp<FloatAttr>(
       operands,
       [](const APFloat &a, const APFloat &b) { return llvm::maximum(a, b); });
 }
 
 //===----------------------------------------------------------------------===//
 // MaxSIOp
 //===----------------------------------------------------------------------===//

 
 OpFoldResult arith::MinFOp::fold(ArrayRef<Attribute> operands) {
   assert(operands.size() == 2 && "minf takes two operands");
 
   // minf(x,x) -> x
   if (getLhs() == getRhs())
     return getRhs();
 
+  // minf(c,x) -> minf(x,c)
+  if (operands.front() && !operands.back()) {
+    std::swap(getOperation()->getOpOperand(0), getOperation()->getOpOperand(1));
+    return getResult();
+  }
+
   return constFoldBinaryOp<FloatAttr>(
       operands,
       [](const APFloat &a, const APFloat &b) { return llvm::minimum(a, b); });
 }
 
 //===----------------------------------------------------------------------===//
 // MinSIOp
 //===----------------------------------------------------------------------===//