[PATCH] D156471: [mlir] Support fast-math friendly constants for identity value

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

> Note: It feels kind of wrong to have to know what the lowering may do, but I don't know what the right (at least short-term) solution is. Long term, we may want a special "neutral element" attribute for the respective ops. I didn't think too much about the implications for that.

Fast-math has consequences that go beyond the neutral element, so I think we need a phased approach. First, we introduce patches like this one to "test the waters" and see what we really want in MLIR. After we learn what went right and what went wrong, we can start formulating a strategy.

Just like we did in LLVM, the defaults for each flag/option will be based on least-surprise, most-safe depending on the optimization level. We don't quite have such a thing for MLIR yet, but as we reinforce the dialects and passes upstream, it's bound to happen sooner or later.

My guess is that we'll tune around what GPU toolchains expect, because most MLIR ends up there, but we need to allow the front-ends to control this somehow for the cases it doesn't (ex. Flang, CPU inference).

Overall, the patch looks good with the inline comments, being "safer" than before, but I'll leave time for others to review.



================
Comment at: mlir/include/mlir/Dialect/Arith/IR/Arith.h:134
+                               OpBuilder &builder, Location loc,
+                               bool useOnlyFiniteValue);
 
----------------
If we eventually add some/all of the fast-math flags from LLVM, we may want this to be a bitfield struct or something.

Not necessarily for this patch, but it's good to start thinking about it.


================
Comment at: mlir/include/mlir/Dialect/Arith/IR/Arith.h:147
 Value getIdentityValue(AtomicRMWKind op, Type resultType, OpBuilder &builder,
-                       Location loc);
+                       Location loc, bool useOnlyFiniteValue);
 
----------------
Can we make these getters to set a default value? then we don't need to override on all calls, only when we know we need, for instance, when the front-end / pass knows what it's doing.


================
Comment at: mlir/lib/Dialect/Arith/IR/ArithOps.cpp:2360
+    APFloat identity = useOnlyFiniteValue
+                           ? APFloat::getSmallest(semantic, /*Negative=*/true)
+                           : APFloat::getInf(semantic, /*Negative=*/true);
----------------
Later we could discuss if it makes sense to expand fast-math logic into `APFloat`, so that `getSmallest`/`getLargest` would "know what to do" against the fast-math flags.


================
Comment at: mlir/test/Dialect/Linalg/transform-op-decompose.mlir:213
 // CHECK-DAG:        %[[D1:.+]] = tensor.empty() : tensor<2x16xf32>
-// CHECK-DAG:        %[[CST:.+]] = arith.constant 0xFF800000 : f32
+// CHECK-DAG:        %[[CST:.+]] = arith.constant -1.401300e-45 : f32
 // CHECK:        %[[D2:.+]] = linalg.fill ins(%[[CST]] : f32) outs(%[[D1]] : tensor<2x16xf32>) -> tensor<2x16xf32>
----------------
With no way to currently pass the flag through `mlir-opt`, it's hard to test the `inf` case...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156471/new/

https://reviews.llvm.org/D156471