zahiraam added inline comments.
================ Comment at: clang/lib/CodeGen/CGCall.cpp:1865 FuncAttrs.addAttribute("approx-func-fp-math", "true"); - if ((LangOpts.FastMath || - (!LangOpts.FastMath && LangOpts.AllowFPReassoc && - LangOpts.AllowRecip && !LangOpts.FiniteMathOnly && - LangOpts.NoSignedZero && LangOpts.ApproxFunc)) && - LangOpts.getDefaultFPContractMode() != LangOptions::FPModeKind::FPM_Off) + if (LangOpts.UnsafeFPMath && + (LangOpts.getDefaultFPContractMode() == ---------------- michele.scandale wrote: > michele.scandale wrote: > > I've found quite confusing the `(FastMath || (!FastMath && ... ))`. > > > > Using directly `UnsafeFPMath` seems more compact, however it also causes to > > taking into account the value for `MathErrno` -- which it might be not > > relevant. > > > > If `MathErrno` shouldn't affect this, then I would rewrite this as: > > ``` > > if (LangOpts.AllowFPReassoc && LangOpts.AllowRecip && > > LangOpts.NoSignedZero && LangOpts.ApproxFunc && > > (LangOpts.getDefaultFPContractMode() == > > LangOptions::FPModeKind::FPM_Fast || > > LangOpts.getDefaultFPContractMode() == > > LangOptions::FPModeKind::FPM_FastHonorPragmas)) > > FuncAttrs.addAttribute("unsafe-fp-math", "true"); > > ``` > > so that only the relevant options are considered and there is no need to > > think about what is implied by `FastMath`. > I do wonder if in > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L3089-L3091 > is actually correct to have `!MathErrno`. In the GCC documentation of > `-funsafe-math-optimizations` I don't see any connection to the > `-fmath-errno` or `-fno-math-errno` options. Interesting! Using on the command line 'funsafe-math-optimization' implies 'math-errno'. But 'funsafe-math-optimizations' is triggered when this is true: !MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && ApproxFunc && !TrappingMath See here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L3089 ================ Comment at: clang/lib/CodeGen/CGCall.cpp:1865 FuncAttrs.addAttribute("approx-func-fp-math", "true"); - if ((LangOpts.FastMath || - (!LangOpts.FastMath && LangOpts.AllowFPReassoc && - LangOpts.AllowRecip && !LangOpts.FiniteMathOnly && - LangOpts.NoSignedZero && LangOpts.ApproxFunc)) && - LangOpts.getDefaultFPContractMode() != LangOptions::FPModeKind::FPM_Off) + if (LangOpts.UnsafeFPMath && + (LangOpts.getDefaultFPContractMode() == ---------------- zahiraam wrote: > michele.scandale wrote: > > michele.scandale wrote: > > > I've found quite confusing the `(FastMath || (!FastMath && ... ))`. > > > > > > Using directly `UnsafeFPMath` seems more compact, however it also causes > > > to taking into account the value for `MathErrno` -- which it might be not > > > relevant. > > > > > > If `MathErrno` shouldn't affect this, then I would rewrite this as: > > > ``` > > > if (LangOpts.AllowFPReassoc && LangOpts.AllowRecip && > > > LangOpts.NoSignedZero && LangOpts.ApproxFunc && > > > (LangOpts.getDefaultFPContractMode() == > > > LangOptions::FPModeKind::FPM_Fast || > > > LangOpts.getDefaultFPContractMode() == > > > LangOptions::FPModeKind::FPM_FastHonorPragmas)) > > > FuncAttrs.addAttribute("unsafe-fp-math", "true"); > > > ``` > > > so that only the relevant options are considered and there is no need to > > > think about what is implied by `FastMath`. > > I do wonder if in > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L3089-L3091 > > is actually correct to have `!MathErrno`. In the GCC documentation of > > `-funsafe-math-optimizations` I don't see any connection to the > > `-fmath-errno` or `-fno-math-errno` options. > Interesting! Using on the command line 'funsafe-math-optimization' implies > 'math-errno'. But 'funsafe-math-optimizations' is triggered when this is true: > !MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && > ApproxFunc && !TrappingMath > > See here: > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L3089 > When the ‘funsafe-math-optimizations’ is used on the command line LangOpts.UnsafeFP is ‘0’. The function attribute ‘funnsafe-fp-math’ is not set. Is that what’s wanted? I would have thought that we want the attribute enabled for unsafe mode. If we call SpecialOperations = AllowFPReassoc && NoSignedZero && AllowRecip && ApproxFunc && !RoundingMath. ‘ffast-math’ = !MathErrno && FiniteMathOnly && SpecialOperations ‘funsafe-math-optimizations’ = MathErrno && !FiniteMathOnly && SpecialOperations (it's true that there is no mention of MathErrno in the GCC doc, but the default is fmath-errno so presumably using this option on the command line implies MathErrno). The function attribute UnsafeFPMath is enabled for (‘ffast-math’ || ‘funsafe-math-optimization’). That will lead to this condition: (SpecialOperations && MathErrNo && "-ffast-math" && -ffp-contract=fast") || (SpecialOperations && "-fmath-errno" && "-ffp-contract=on") . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136786/new/ https://reviews.llvm.org/D136786 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits