[PATCH] D135097: Remove redundant option '-menable-unsafe-fp-math'.

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1865-1869
+    if ((LangOpts.FastMath ||
+         !LangOpts.FastMath && LangOpts.AllowFPReassoc && LangOpts.AllowRecip &&
+             !LangOpts.FiniteMathOnly && LangOpts.NoSignedZero &&
+             LangOpts.ApproxFunc) &&
+        LangOpts.getDefaultFPContractMode() != LangOptions::FPModeKind::FPM_Off)
----------------
michele.scandale wrote:
> michele.scandale wrote:
> > zahiraam wrote:
> > > michele.scandale wrote:
> > > > If I look at the clang driver code, `-funsafe-math-optimizations` is specified on the CC1 command line if
> > > > ```
> > > > !MathErrno && AssociativeMath && ReciprocalMath && !SignedZeros && ApproxFunc && !TrappingMath
> > > > ```
> > > > is true.
> > > > This means that it shouldn't matter the value of the floating point contraction, or whether infs/nans are honored or not.
> > > > 
> > > > Was there another issue that this specific part of the change addresses?
> > > This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated.  In this case the function shouldn't be marked with the unsafe-fp-math attribute.
> > > This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated.  In this case the function shouldn't be marked with the unsafe-fp-math attribute.
> > 
> > If I understand correctly this is because the function attribute `unsafe-fp-math` from backends perspective allow any kind of contraction?
> > If so, shouldn't then this be valid only when the fp-contraction mode is `fast`, given that `on` means that only in-statement contraction is allowed, and clang handles that by generating `llvm.fmuladd` to lower expressions like `a * b + c` in given statement?
> > 
> > What about the `!LangOpts.FiniteMathOnly` part? I'd would think this shouldn't be checked at all as there are separate function attributes for infs and nans.
> > This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated.  In this case the function shouldn't be marked with the unsafe-fp-math attribute.
> 
> 
> > This is to make sure that if we are using -ffast-math or -funsafe-math-optimization along with -ffp-contract=off, fma instructions are not generated.  In this case the function shouldn't be marked with the unsafe-fp-math attribute.
> 
> If I understand correctly this is because the function attribute `unsafe-fp-math` from backends perspective allow any kind of contraction?
That's correct.

> If so, shouldn't then this be valid only when the fp-contraction mode is `fast`, given that `on` means that only in-statement contraction is allowed, and clang handles that by generating `llvm.fmuladd` to lower expressions like `a * b + c` in given statement?
> 
> What about the `!LangOpts.FiniteMathOnly` part? I'd would think this shouldn't be checked at all as there are separate function attributes for infs and nans.

when ffp-contract=on is used with no safe optimizations along with it, the attribute unsafe-fp-math is not set but tryEmitMulAdd will return an FMulAdd. Please keep in mind that this logic hasn't really changed. The only thing that was done was to explicitly expand the operations of unsafe optimizations in the original condition.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135097