michele.scandale 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:
> 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.


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

Reply via email to