zahiraam 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:
> > 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
 


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




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

Reply via email to