[PATCH] D103233: [Matrix] Preserve existing fast-math flags during lowering

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update! A few more suggestions about dealing with the `FMF` object.



================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:407
+private:
+  static Optional<FastMathFlags> getFastMathFlags(Instruction *Inst) {
+    if (isa<FPMathOperator>(*Inst)) {
----------------
Instead of returning `None`, could we just return a FastMathFlags object with all options set the false (which is the default) ?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1183
+      Builder.setFastMathFlags(FMF);
+      AllowContraction |= FMF.allowContract();
+    }
----------------
I think this should be the other way around, if `AllowContractEnabled` we could set `AllowContraction` on `FMF`. This default could be set in `getFastMathFlags`?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1689
       case Instruction::FAdd:
-        return Builder.CreateFAdd(LHS, RHS);
+        return Builder.CreateFAddFMF(LHS, RHS, Inst);
       case Instruction::FMul:
----------------
Can we instead use the FMF object in those versions instead? `AllowContraction` option should probably also add the `contract` flag to those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103233