Include math-errno with fast-math

[zahiraam]

Summary

Take math-errno into account with '#pragma float_control(precise,on)' and 'attribute__((optnone)).

https://github.com/llvm/llvm-project/issues/62098

[andrew.w.kaylor]

clang/include/clang/Basic/FPOptions.def

30

Does this mean MathErrno is tracked in both LangOpts and FPOptions?

clang/lib/CodeGen/CGBuiltin.cpp

2248

You should add a comment here explaining what this is doing. I don't think it's obvious apart from the description of this patch.

clang/lib/CodeGen/CGCall.cpp

2395

I don't understand what this is doing.

clang/lib/CodeGen/CodeGenModule.h

611

Why is this a module level flag, and why does it default to true?

clang/test/CodeGen/math-errno.c

3

Isn't math-errno true by default when fast-math isn't used?

50

I think the 'afn' flag here is a problem. The backend has no concept of errno, so 'afn' will be treated as allowing the function to be replaced.

[andrew.w.kaylor]

clang/test/CodeGen/math-errno.c

34

Can you add a runline with -O0. That should prevent all instances of the intrinsics, right?

[zahiraam]

clang/include/clang/Basic/FPOptions.def

30

Yes. So that we can check that if it's overridden by a pragma or an attribute. I don't see another way.

clang/test/CodeGen/math-errno.c

34

Yes.

50

Added all the commands that are triggered by fast-math in the RUN line command.

[andrew.w.kaylor]

clang/lib/CodeGen/CGBuiltin.cpp

2249

Since we handle "#pragma float_control" for all targets, we should be consistent. I don't think the reference to MSVC here is useful.

I'm not sure why MSVC thinks it needs to call the sqrtf() function at O1, but I don't necessarily think we should follow that lead. If we generate the intrinsic here, the optimizer will make its own decisions about O1 versus O2, etc. There's no reason that errno should be preserved at O1 if it's been disabled with -fno-math-errno or we're enabled fast-math.

2263

I'm confused by this condition. If MathErrnoOverride is true, don't we need to set errno regardless of the optimization level or fast-math setting?

2266

Why do we need to check FastMath if the function is marked as optnone?

2284

Based on the comment above, if the function call is marked as const, then it will never set errno, so I don't think we need to check EmitIntrinsic in that case.

clang/test/CodeGen/math-errno.c

28

Shouldn't the precise pragma remove the nofpcalss(nan inf) attributes? The definition of setFPPreciseEnabled looks like it's trying to do that. Maybe we need another patch to handle that?

40

Again, this may be beyond the scope of your change, but it seems like '#pragma float_control(precise, off) should lead to fast-math flags being set on this call.

43

I expected the intrinsic to be called in this case. Do you know why it isn't? I believe it is without your change.

46

I didn't expect to see the intrinsic in this case. If we use the intrinsic, the x86-backend will generate sqrtss instead of the function call, and I think that is wrong.

56

Again, I didn't expect the intrinsic with optnone in any of these cases.

[zahiraam]

clang/test/CodeGen/math-errno.c

3

Yes, that's correct. This RUN line simulates compilation at -O2. The following cc1 options are triggered: -fmath-errno -ffp-contract=on
-fno-rounding-math -O2. That's what I am using here.

28

Shouldn't the precise pragma remove the nofpcalss(nan inf) attributes? The definition of setFPPreciseEnabled looks like it's trying to do that. Maybe we need another patch to handle that?

I will make a note of it and work on it in a subsequent patch.

40

Again, this may be beyond the scope of your change, but it seems like '#pragma float_control(precise, off) should lead to fast-math flags being set on this call.

Yes. Making a note of it.

[zahiraam]

Review anyone please? Thanks.

[aaron.ballman]

clang/include/clang/Basic/FPOptions.def

29–30

Shouldn't this one be Float16ExcessPrecision? (Are we missing test coverage that would have caught that?)

clang/include/clang/Basic/LangOptions.h

853

Everything else does !Value; is it intentional that you're using Value instead?

clang/lib/CodeGen/CGBuiltin.cpp

2288–2295

I think this has gotten sufficiently complex that it might be worth splitting the logic out a bit more:

bool Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone;
if (!Optimize) Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone;
... and so on ...

WDYT?

[zahiraam]

clang/include/clang/Basic/FPOptions.def

29–30

oops! the test would be for the BFloat16ExcessPrecision, right?

clang/include/clang/Basic/LangOptions.h

853

Yes. I want to set the value of math-errno to the value of the pragma in the source.

[aaron.ballman]

clang/include/clang/Basic/FPOptions.def

29–30

Yup -- that can be handled in a separate patch as it doesn't really have much to do with this one -- just something I noticed as a drive-by.

clang/include/clang/Basic/LangOptions.h

853

Hmm, okay. Making sure I understand the logic, please excuse me if this is a dumb question. :-) So the idea here is that if fp_precise is on, we have to honor math errno, and if fp_precise is off, we can ignore math errno?

clang/lib/CodeGen/CGBuiltin.cpp

2260

Is the part about -O2 accurate? We just check not -O0 in the code.

[zahiraam]

clang/include/clang/Basic/LangOptions.h

853

Correct.

[zahiraam]

clang/lib/CodeGen/CGBuiltin.cpp

2288–2295

I understand I need to split the logic, but I don't get your proposal here. Do you mean?

bool Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone;
if (!Optimize) Optimize = !getLangOpts().MathErrno && !ErrnoOverriden && !OptNone;
... and so on ...

[aaron.ballman]

clang/lib/CodeGen/CGBuiltin.cpp

2288–2295

Oops, yeah, that a was copy pasta mistake on my part. You've got the right idea -- mostly just split up the logic and add comments explaining why the predicates exist.

[zahiraam]

clang/lib/CodeGen/CGBuiltin.cpp

2288–2295

Changed the name of the variable you proposed. Is that OK?

[aaron.ballman]

clang/lib/CodeGen/CGBuiltin.cpp

2288–2295

Totally fine by me, but there's some more logic that you can strip out of the if statement, and it'd help me out if there were some comments explaining why each of these blocks might disable generating intrinsics.

[aaron.ballman]

clang/lib/CodeGen/CGBuiltin.cpp

2285–2287

[aaron.ballman]

clang/docs/LanguageExtensions.rst

4698 ↗

(On Diff #545666)

4702 ↗

(On Diff #545666)

clang/docs/UsersManual.rst

1727 ↗

(On Diff #545666)

Can you re-flow this to 80 columns?

[zahiraam]

clang/docs/LanguageExtensions.rst

4702 ↗

(On Diff #545666)

At least I was consistent :-)

clang/docs/UsersManual.rst

1727 ↗

(On Diff #545666)

Did the same for the rest of the text too. Is that OK?

[aaron.ballman]

clang/docs/UsersManual.rst

1727 ↗

(On Diff #545666)

Yeah, that's not the end of the world (if it was touching the whole file, that should be done as an NFC change outside of this patch, but these changes are related enough I think it's fine to do in this patch).

[aaron.ballman]

LGTM, but please add a release note describing the changes when you land them.

[zahiraam]

In D151834#4639536, @aaron.ballman wrote:

LGTM, but please add a release note describing the changes when you land them.

@aaron.ballman thanks for the reviews. Added some note in the RN, let me know if that is enough.

[aaron.ballman]

In D151834#4639600, @zahiraam wrote:

In D151834#4639536, @aaron.ballman wrote:

LGTM, but please add a release note describing the changes when you land them.

@aaron.ballman thanks for the reviews. Added some note in the RN, let me know if that is enough.

Release note looks great, thank you!

[zahiraam]

@andrew.w.kaylor Would you mind taking a last look at this before I can push this, please? I got the thumbs up from Aaron.

[andrew.w.kaylor]

lgtm

[thakis]

This breaks check-clang on macOS: http://45.33.8.238/macm1/68762/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[zahiraam]

In D151834#4642195, @thakis wrote:

This breaks check-clang on macOS: http://45.33.8.238/macm1/68762/step_7.txt

Please take a look and revert for now if it takes a while to fix.

I have a fix for it here: https://reviews.llvm.org/D159486

[uabelho]

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

[aaron.ballman]

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

[zahiraam]

In D151834#4644373, @aaron.ballman wrote:

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

[aaron.ballman]

In D151834#4644375, @zahiraam wrote:

In D151834#4644373, @aaron.ballman wrote:

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

[zahiraam]

In D151834#4644378, @aaron.ballman wrote:

In D151834#4644375, @zahiraam wrote:

In D151834#4644373, @aaron.ballman wrote:

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

[aaron.ballman]

In D151834#4644391, @zahiraam wrote:

In D151834#4644378, @aaron.ballman wrote:

In D151834#4644375, @zahiraam wrote:

In D151834#4644373, @aaron.ballman wrote:

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.

[zahiraam]

In D151834#4644394, @aaron.ballman wrote:

In D151834#4644391, @zahiraam wrote:

In D151834#4644378, @aaron.ballman wrote:

In D151834#4644375, @zahiraam wrote:

In D151834#4644373, @aaron.ballman wrote:

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.

I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

[zahiraam]

In D151834#4644422, @zahiraam wrote:

In D151834#4644394, @aaron.ballman wrote:

In D151834#4644391, @zahiraam wrote:

In D151834#4644378, @aaron.ballman wrote:

In D151834#4644375, @zahiraam wrote:

In D151834#4644373, @aaron.ballman wrote:

In D151834#4643925, @uabelho wrote:

Hi @zahiraam ,

I have a couple of downstream testcases that fail with this patch.
Before

> clang bbi-86364.c -lm -O3
> ./a.out

passed but with the patch the assert in the program fails:

a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.

Is this as expected?

bbi-86364.c165 BDownload

This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?

I haven't looked at it as I saw that the comment has been deleted. Let me look into it.

Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4

Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?

Yeah, I think this is a bug with your patch, but I'm not certain where. I suspect the changes to CGBuiltin.cpp are what are causing this issue -- CC @efriedma and @rjmccall as they might have ideas.

I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

This looks like it's happening only for -Oi (i>=1) and on Linux only?

[uabelho]

In D151834#4644692, @zahiraam wrote:

[...]

I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365

This looks like it's happening only for -Oi (i>=1) and on Linux only?

Yes I haven't seen it with -O0. I also haven't seen it for my out-of-tree target, only for linux/X86.

[uabelho]

What's the way forward here? Revert if it's unclear what to do?

[zahiraam]

In D151834#4645866, @uabelho wrote:

What's the way forward here? Revert if it's unclear what to do?

We have a fix for it. By eod a PR will be created. Thanks.

[uabelho]

In D151834#4645910, @zahiraam wrote:

In D151834#4645866, @uabelho wrote:

What's the way forward here? Revert if it's unclear what to do?

We have a fix for it. By eod a PR will be created. Thanks.

Ok, nice. Thanks!

clang/include/clang/Basic/FPOptions.def

 OPTION(AllowFPReassociate, bool, 1, AllowFEnvAccess)
 OPTION(NoHonorNaNs, bool, 1, AllowFPReassociate)
 OPTION(NoHonorInfs, bool, 1, NoHonorNaNs)
 OPTION(NoSignedZero, bool, 1, NoHonorInfs)
 OPTION(AllowReciprocal, bool, 1, NoSignedZero)
 OPTION(AllowApproxFunc, bool, 1, AllowReciprocal)
 OPTION(FPEvalMethod, LangOptions::FPEvalMethodKind, 2, AllowApproxFunc)
 OPTION(Float16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, FPEvalMethod)
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)
 #undef OPTION

clang/include/clang/Basic/LangOptions.h

public:
 
   void setFPPreciseEnabled(bool Value) {
     setAllowFPReassociateOverride(!Value);
     setNoHonorNaNsOverride(!Value);
     setNoHonorInfsOverride(!Value);
     setNoSignedZeroOverride(!Value);
     setAllowReciprocalOverride(!Value);
     setAllowApproxFuncOverride(!Value);
+    setMathErrnoOverride(Value);
     if (Value)
       /* Precise mode implies fp_contract=on and disables ffast-math */
       setAllowFPContractWithinStatement();
     else
       /* Precise mode disabled sets fp_contract=fast and enables ffast-math */
       setAllowFPContractAcrossStatement();
   }
 

clang/lib/CodeGen/CGBuiltin.cpp

if (getTarget().getTriple().isPPC64() &&
     BuiltinID = mutateLongDoubleBuiltin(BuiltinID);
 
   // If the builtin has been declared explicitly with an assembler label,
   // disable the specialized emitting below. Ideally we should communicate the
   // rename in IR, or at least avoid generating the intrinsic calls that are
   // likely to get lowered to the renamed library functions.
   const unsigned BuiltinIDIfNoAsmLabel =
       FD->hasAttr<AsmLabelAttr>() ? 0 : BuiltinID;
 
+  // MSVC takes into account math-errno when compiling at -O2 level in
+  // the presence of a '#pragma float_control precise(on/off)'.
+  // ErrnoWithO2OrFastMath checks for that.
+  // When compiling with 'ffast-math' check if 'math-errno' is overriden, or
+  // if the '__attribute__((optnone)) is used.
+  // ErrnoWithO2OrFastMath and OptNoneWithFastMath check for that.
+  // In both of these cases don't emit intrinsics.
+  // At -O0 level, no intrinsic should be generated.
+  bool MathErrnoOverrride = false;
+  if (E->hasStoredFPFeatures()) {
+    FPOptionsOverride OP = E->getFPFeatures();
+    if (OP.hasMathErrnoOverride())
+      MathErrnoOverrride = OP.getMathErrnoOverride();
+  }
+  bool ErrnoWithO2OrFastMath =
+      MathErrnoOverrride && (CGM.getCodeGenOpts().OptimizationLevel == 2 ||
+                             CGM.getLangOpts().FastMath);
+  bool OptNoneWithFastMath =
+      CurFuncDecl->hasAttr<OptimizeNoneAttr>() && CGM.getLangOpts().FastMath;
+  bool EmitIntrinsic =
+      ((CGM.getCodeGenOpts().OptimizationLevel != 0) &&
+                       !OptNoneWithFastMath && !ErrnoWithO2OrFastMath);
+
   // There are LLVM math intrinsics/instructions corresponding to math library
   // functions except the LLVM op will never set errno while the math library
   // might. Also, math builtins have the same semantics as their math library
   // twins. Thus, we can transform math library and builtin calls to their
   // LLVM counterparts if the call is marked 'const' (known to never set errno).
   // In case FP exceptions are enabled, the experimental versions of the
   // intrinsics model those.
   bool ConstWithoutErrnoAndExceptions =
       getContext().BuiltinInfo.isConstWithoutErrnoAndExceptions(BuiltinID);
   bool ConstWithoutExceptions =
       getContext().BuiltinInfo.isConstWithoutExceptions(BuiltinID);
-  if (FD->hasAttr<ConstAttr>() ||
+
+  if ((FD->hasAttr<ConstAttr>() && EmitIntrinsic) || EmitIntrinsic ||
        ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-       (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno)))) {
+         (!ConstWithoutErrnoAndExceptions ||
+        (!getLangOpts().MathErrno && EmitIntrinsic)))) {
     switch (BuiltinIDIfNoAsmLabel) {
     case Builtin::BIceil:
     case Builtin::BIceilf:
     case Builtin::BIceill:
     case Builtin::BI__builtin_ceil:
     case Builtin::BI__builtin_ceilf:
     case Builtin::BI__builtin_ceilf16:
     case Builtin::BI__builtin_ceill:
     case Builtin::BI__builtin_ceilf128:
       return RValue::get(emitUnaryMaybeConstrainedFPBuiltin(*this, E,
                                    Intrinsic::ceil,
                                    Intrinsic::experimental_constrained_ceil));
 
     case Builtin::BIcopysign:
     case Builtin::BIcopysignf:
     case Builtin::BIcopysignl:

clang/lib/CodeGen/CGCall.cpp

static llvm::FPClassTest getNoFPClassTestMask(const LangOptions &LangOpts) {
   llvm::FPClassTest Mask = llvm::fcNone;
   if (LangOpts.NoHonorInfs)
     Mask |= llvm::fcInf;
   if (LangOpts.NoHonorNaNs)
     Mask |= llvm::fcNan;
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+                                          CGCalleeInfo CalleeInfo,
+                                          llvm::AttributeList &Attrs) {
+  if (Attrs.getMemoryEffects().getModRef() == llvm::ModRefInfo::NoModRef) {
+    Attrs = Attrs.removeFnAttribute(getLLVMContext(), llvm::Attribute::Memory);
+    llvm::Attribute MemoryAttr = llvm::Attribute::getWithMemoryEffects(
+        getLLVMContext(), llvm::MemoryEffects::writeOnly());
+    Attrs = Attrs.addFnAttribute(getLLVMContext(), MemoryAttr);
+  }
+}
+
 /// Construct the IR attribute list of a function or call.
 ///
 /// When adding an attribute, please consider where it should be handled:
 ///
 ///   - getDefaultFunctionAttributes is for attributes that are essentially
 ///     part of the global target configuration (but perhaps can be
 ///     overridden on a per-function basis).  Adding attributes there
 ///     will cause them to also be set in frontends that build on Clang's
void CodeGenModule::ConstructAttributeList(StringRef Name,
   // The attributes can come from:
   // * LangOpts: -ffreestanding, -fno-builtin, -fno-builtin-<name>
   // * FunctionDecl attributes: __attribute__((no_builtin(...)))
   addNoBuiltinAttributes(FuncAttrs, getLangOpts(), NBA);
 
   // Collect function IR attributes based on global settiings.
   getDefaultFunctionAttributes(Name, HasOptnone, AttrOnCallSite, FuncAttrs);
 
   // Override some default IR attributes based on declaration-specific
   // information.
   if (TargetDecl) {
     if (TargetDecl->hasAttr<NoSpeculativeLoadHardeningAttr>())
       FuncAttrs.removeAttribute(llvm::Attribute::SpeculativeLoadHardening);
     if (TargetDecl->hasAttr<SpeculativeLoadHardeningAttr>())
       FuncAttrs.addAttribute(llvm::Attribute::SpeculativeLoadHardening);
     if (TargetDecl->hasAttr<NoSplitStackAttr>())
       FuncAttrs.removeAttribute("split-stack");
#endif
   // Compute the calling convention and attributes.
   unsigned CallingConv;
   llvm::AttributeList Attrs;
   CGM.ConstructAttributeList(CalleePtr->getName(), CallInfo,
                              Callee.getAbstractInfo(), Attrs, CallingConv,
                              /*AttrOnCallSite=*/true,
                              /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl)) {
     if (FD->hasAttr<StrictFPAttr>())
       // All calls within a strictfp function are marked strictfp
       Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::StrictFP);
 
+    // If -ffast-math is enabled and the function is guarded by an '__attribute__((optnone))
+    // adjust the memory attribute so the BE emits the library call instead of the intrinsic.
+    if (FD->hasAttr<OptimizeNoneAttr>() && getLangOpts().FastMath)
+      CGM.AdjustMemoryAttribute(CalleePtr->getName(), Callee.getAbstractInfo(),
+                                Attrs);
+  }
   // Add call-site nomerge attribute if exists.
   if (InNoMergeAttributedStmt)
     Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoMerge);
 
   // Add call-site noinline attribute if exists.
   if (InNoInlineAttributedStmt)
     Attrs = Attrs.addFnAttribute(getLLVMContext(), llvm::Attribute::NoInline);
 

clang/lib/CodeGen/CodeGenModule.h

CodeGenModule(ASTContext &C, IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
                 const PreprocessorOptions &ppopts,
                 const CodeGenOptions &CodeGenOpts, llvm::Module &M,
                 DiagnosticsEngine &Diags,
                 CoverageSourceInfo *CoverageInfo = nullptr);
 
   ~CodeGenModule();
 
   void clear();
 
   /// Finalize LLVM code generation.
   void Release();
 
   /// Return true if we should emit location information for expressions.
   bool getExpressionLocationsEnabled() const;
 
   /// Return a reference to the configured Objective-C runtime.
   CGObjCRuntime &getObjCRuntime() {
public:
   /// contribute to the function attributes and calling convention.
   /// \param Attrs [out] - On return, the attribute list to use.
   /// \param CallingConv [out] - On return, the LLVM calling convention to use.
   void ConstructAttributeList(StringRef Name, const CGFunctionInfo &Info,
                               CGCalleeInfo CalleeInfo,
                               llvm::AttributeList &Attrs, unsigned &CallingConv,
                               bool AttrOnCallSite, bool IsThunk);
 
+  /// Adjust Memory attribute to ensure that the BE gets the right attribute
+  // in order to generate the library call or the intrinsic for the function
+  // name 'Name'.
+  void AdjustMemoryAttribute(StringRef Name,
+                             CGCalleeInfo CalleeInfo, llvm::AttributeList &Attrs);
+
   /// Adds attributes to F according to our CodeGenOptions and LangOptions, as
   /// though we had emitted it ourselves.  We remove any attributes on F that
   /// conflict with the attributes we add here.
   ///
   /// This is useful for adding attrs to bitcode modules that you want to link
   /// with but don't control, such as CUDA's libdevice.  When linking with such
   /// a bitcode library, you might want to set e.g. its functions'
   /// "unsafe-fp-math" attribute to match the attr of the functions you're

clang/test/CodeGen/math-errno.c

+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -ffp-contract=on -fno-rounding-math -O2 -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+
+// -ffast-math
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -menable-no-infs -menable-no-nans -fapprox-func \
+// RUN: -funsafe-math-optimizations -fno-signed-zeros -mreassociate \
+// RUN: -freciprocal-math -ffp-contract=fast -fno-rounding-math -ffast-math \
+// RUN: -ffinite-math-only -ffast-math -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefix=FAST
+
+// -O0
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -ffp-contract=on -fno-rounding-math -O2 -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefix=NOOPT
+
+#pragma float_control(precise,on)
+float f1(float x) {
+  return sqrtf(x);
+}
+
+// CHECK-LABEL: define dso_local float @f1
+// CHECK: tail call float @sqrtf(float noundef {{.*}}) #[[ATTR4_O2:[0-9]+]]
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f1
+// FAST: call fast nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) {{.*}}) #[[ATTR3_FAST:[0-9]+]]
+//
+// NOOPT-LABEL: define dso_local float @f1
+// NOOPT: tail call float @sqrtf(float noundef {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+
+#pragma float_control(precise,off)
+float f2(float x) {
+  return sqrtf(x);
+}
+
+// CHECK-LABEL: define dso_local float @f2
+// CHECK: tail call float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f2
+// FAST: call fast nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) {{.*}}) #[[ATTR3_FAST]]
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: tail call float @llvm.sqrt.f32(float {{.*}})
+
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f3
+// FAST: call fast nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) {{.*}}) #[[ATTR4_FAST:[0-9]+]]
+
+// NOOPT-LABEL: define dso_local float @f3
+// NOOPT: call float @llvm.sqrt.f32(float {{.*}})
+
+// CHECK: [[ATTR4_O2]] = { nounwind willreturn memory(none) }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind willreturn memory(none) }