andrew.w.kaylor 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:
> 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?
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
