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