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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
