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

[Aleksei Nurmukhametov]

================
@@ -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);
----------------
nurmukhametov wrote:

> Answering the question of whether this change here is necessary, 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).

I agree that it doesn't seem to be clearly defined. I changed the code and the test in favor of the second option as suggested by @bjacob 

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