[PATCH] D136786: Fix `unsafe-fp-math` attribute emission.

[Michele Scandale via Phabricator via cfe-commits]

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() ==
----------------
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`.


================
Comment at: clang/test/CodeGen/func-attr.c:5
 // RUN: %clang -c -target x86_64 -funsafe-math-optimizations \
-// RUN: -emit-llvm -S -o - %s | FileCheck %s
+// RUN: -fno-math-errno -ffp-contract=fast -emit-llvm -S -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-UNSAFE
+
----------------
See comment about `MathErrno` in `CGCall.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786