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

Michele Scandale via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 27 14:52:23 PDT 2022


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() ==
----------------
zahiraam wrote:
> 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") .
> 
> 
The compiler driver has a default value for `MathErrno` which depends on the toolchain properties. When `-funsafe-math-optimizations` is processed `MathErrno` is not affected, but then the compiler driver specify `-funsafe-math-optimizations` to the CC1 command line only if `MathErrno` is false.
The way the compiler driver mutate the floating point options state when processing `-funsafe-math-optimizations` seems to match the GCC documentation, but then the condition for the forwarding expect something more.
It is not clear to me if the intention is to match GCC documented behavior, or if instead the Clang definition of `-funsafe-math-optimizations` implies `-fno-math-errno`.


================
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:
> zahiraam wrote:
> > 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") .
> > 
> > 
> The compiler driver has a default value for `MathErrno` which depends on the toolchain properties. When `-funsafe-math-optimizations` is processed `MathErrno` is not affected, but then the compiler driver specify `-funsafe-math-optimizations` to the CC1 command line only if `MathErrno` is false.
> The way the compiler driver mutate the floating point options state when processing `-funsafe-math-optimizations` seems to match the GCC documentation, but then the condition for the forwarding expect something more.
> It is not clear to me if the intention is to match GCC documented behavior, or if instead the Clang definition of `-funsafe-math-optimizations` implies `-fno-math-errno`.
>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.

I believe this is caused by `MathErrno` state in the compiler driver being not affected when processing `-funsafe-math-optimizations`, but considered for the forwarding to the CC1.

>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") .

In GCC `-funsafe-math-optimizations` does not seem to affect the state of math-errno -- see https://godbolt.org/z/jzW6zqxeM and https://godbolt.org/z/axPb63z3d.

I'm not following entirely, but `-funsafe-math-optimizations` is just a subset of `-ffast-math`, so checking only the properties that contribute to `-funsafe-math-optimizations` should be enough. I think it is way simpler to reason in these terms than enumerating all the possible scenarios.

Now, in https://reviews.llvm.org/D135097#3885466 you said that additional condition on the floating point contraction mode was intended due to the semantic of the `unsafe-fp-math` from the backends perspective -- i.e. if `"unsafe-fp-math"="true"` is specified as a function attribute, then the backend can perform any form of contraction on that function. As shown in the example in the description if you only check that `LangOpts.getDefaultFPContractMode() != LangOptions::FPModeKind::FPM_Off`, then you get unexpected contraction. Therefore if the intention is to ensure the behavior is correct, then you can only set `"unsafe-fp-math"="true"` only if you have fast contraction mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786



More information about the cfe-commits mailing list