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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked 5 inline comments as done.
qcolombet added inline comments.


================
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);
 
----------------
rengolin wrote:
> qcolombet wrote:
> > rengolin wrote:
> > > 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.
> > Yes we could. I decided against it because I wanted people to think about this problem when they use this API.
> > Now, since this may be a temporary solution, it may be better to limit the changes.
> > 
> > WDYT?
> > I wanted people to think about this problem when they use this API.
> 
> This works for people adding new code, not the existing code that is being changed with this patch, which the original authors are probably not going to look back.
> 
> > it may be better to limit the changes.
> 
> Also, if the RFC linked goes ahead, the argument will change soon and lead to more mechanical changes.
> 
> I'd keep the changes minimal (default value) for now and enter the RFC discussion for the long term strategy.
Used default value that keeps the old behavior.


================
Comment at: mlir/lib/Dialect/Arith/IR/ArithOps.cpp:2408
+std::optional<TypedAttr>
+mlir::arith::getNeutralElement(Operation *op, bool useOnlyFiniteValue) {
   std::optional<AtomicRMWKind> maybeKind =
----------------
qcolombet wrote:
> Hardcode84 wrote:
> > qcolombet wrote:
> > > Hardcode84 wrote:
> > > > While we need to pass flag explicitly to `AtomicRMWKind` version, for op version we can take fastmath flag from op itself (which already have fastmath flags support I believe).
> > > I think the fastmath flags are only on arith operations, not on the generic ones.
> > > I.e., I'm not sure this is generally possible.
> > Current `getNeutralElement(op)` implementation supports only arith ops now, so I don't see problem here.
> Good point. Let me give it a try then.
Please take a look @Hardcode84 


================
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>
----------------
qcolombet wrote:
> rengolin wrote:
> > With no way to currently pass the flag through `mlir-opt`, it's hard to test the `inf` case...
> Good point, maybe we'll want a sort of index-width option equivalent for fast math handling.
Added a test through the reduction split test following @Hardcode84 recommendation of using the fastmath flag attached to the operation when using `getIdentityElement`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156471