[Mlir-commits] [mlir] [mlir][math] Propagate fast math attrs in AlgebraicSimplification (PR #166802)

[Benoit Jacob]

================
@@ -43,6 +43,10 @@ PowFStrengthReduction::matchAndRewrite(math::PowFOp op,
                                        PatternRewriter &rewriter) const {
   Location loc = op.getLoc();
   Value x = op.getLhs();
+  arith::FastMathFlags fmf = op.getFastmathAttr().getValue();
+  arith::FastMathFlags intermediateFmf = arith::bitEnumClear(
+      fmf, arith::FastMathFlags::reassoc | arith::FastMathFlags::contract |
+               arith::FastMathFlags::arcp);
----------------
bjacob wrote:

(Preamble: I think that llvm's fast-math flags are flawed and need a complete rewrite, see https://github.com/iree-org/iree/issues/19743, but I've stayed out of this as I heard noises that others were apparently rethinking it already; maybe Joshua, who Jakub already requested a review from here, has more context on the big picture).

Answering the question of whether this change here is necessary or at least is correct, depends on fine detail of the semantics of these flags. Specifically, the alternative is:
1. Do {reassoc, contract, arcp} apply only literally to the op itself?
2. Or do they also apply to all ops in any rewrite of the op that is legal under its fast-math semantics?

Reading https://llvm.org/docs/LangRef.html#rewrite-based-flags, I can't honestly say that this seems clearly defined. However, as a matter of my mental model and what I believe would be a step towards usefulness for LLVM fast-math flags, I would suggest resolving that ambiguity towards 2. in the above alternative. Rationale: this enables more useful optimizations in cases where the source code had already opted into fast-math rewrites, this is an easier mental model ("transitivity of rewritability") and the compiler code is simpler (evidenced by removing the need for this part of the diff in this PR).

https://github.com/llvm/llvm-project/pull/166802