[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() ==
----------------
andrew.w.kaylor wrote:
> michele.scandale wrote:
> > 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.
> I'd really like to see us move away from fast-math and unsafe-fp-math and encourage use of individual semantic modes. Both fast-math and unsafe-fp-math are composite settings and I'm sure there are users that depend on them, just as there are optimizations that are checking the function attribute. We need to support the command-line options for the users who have existing build systems using it. We can start fixing the uses within clang and LLVM wherever we can do so without changing behavior (or maybe even changing behavior just a little in reasonable ways).
> 
> The main thing I have in mind to start with is the interface between the driver and the front end. We have code in [[ https://github.com/llvm/llvm-project/blob/1d4a57bd12783ff98faed630e800e2c3675dd4d6/clang/lib/Driver/ToolChains/Clang.cpp#L3144 | clang/lib/Driver/ToolChains/Clang.cpp ]] that does a bunch of analysis of various options to decide whether or not to pass -ffast-math to the compiler invocation. I don't understand why it can't just pass the individual semantic mode flags and let the front end figure out the composite settings if it needs them. Then if we can ever wean people off of the composite settings, we can get rid of that. I don't know if there is similar handling in the other Driver tool chains files, but I'd view it the same way.
> 
> The composite options are much less useful to users if they don't have consistent meanings across all compilers that support them. I think our handling of them should be consistent with gcc. We should also consider OpenCL's -cl-unsafe-math-optimizations option. At a glance, it looks to me like it's consistent with the gcc meaning.
> 
> I think we should drop math-errno from our definition of unsafe-fp-math. The errno variable is a very C-specific concept and it shouldn't creep into language-independent handling. The optimizer has no business knowing anything about math errno or being able to deduce it from any setting in the IR. The optimizer only needs to know "I'm allowed to replace this function call" or "I'm not allowed to replace this function call". Setting math errno is a side effect of the library call and the optimizer only needs to know that the call has unmodeled side effects. Generally we handle this by choosing the LLVM intrinsics when the side effects aren't needed, right?
>The main thing I have in mind to start with is the interface between the driver and the front end. We have code in clang/lib/Driver/ToolChains/Clang.cpp that does a bunch of analysis of various options to decide whether or not to pass -ffast-math to the compiler invocation. I don't understand why it can't just pass the individual semantic mode flags and let the front end figure out the composite settings if it needs them. Then if we can ever wean people off of the composite settings, we can get rid of that. I don't know if there is similar handling in the other Driver tool chains files, but I'd view it the same way.

Yes, I agree 100% with this.
I would expect that the CC1 to only expose flags to control each individual low level floating point options, and let the compiler driver (which represents the public interface with the user) to one responsible of translating the high level aggregate/options in a combination of the low-level options.
For example, I really would like that when the compiler driver processes
```
clang -ffast-math -c foo.c
```
would generate something like the following as a CC1 command line
```
clang -cc1 \
  -fassociative-math \ # controls the `reassoc` FMF
  -freciprocal-math \ # controls the `arcp` FMF
  -fapprox-func \ # controls the `afn` FMF
  -fno-signed-zeros \ # controls the `nsz` FMF
  -fno-honor-infs \ # controls the `ninf` FMF
  -fno-honor-nans \ # controls the `nnan` FMF
  -ffp-contract=fast \ # controls the `contract` FMF
  -fno-rounding-math \ 
  -ffp-exception-behavior=ignore \
  -fno-math-errno
```

I'm happy to restructure the condition to set `"unsafe-fp-math"="true"` to not use `LangOpts.UnsafeFPMath`, but I'd also expect not to use `LangOpts.FastMath` either.

> I think we should drop math-errno from our definition of unsafe-fp-math. 

I'm happy to fix the Clang driver code to remove the `!MathErrno` in the check that guards forwarding `-funsafe-math-optimizations` to the CC1. I would think this should be fixed separately from this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136786