[PATCH] D151834: Include math-errno with fast-math

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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?
>>>
>>> F29200339: bbi-86364.c 
>>
>> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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?
>>
>> F29200339: bbi-86364.c 
>
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?
>
> F29200339: bbi-86364.c 

 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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?

 F29200339: bbi-86364.c 
>>>
>>> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?
>>>
>>> F29200339: bbi-86364.c 
>>
>> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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?
>>
>> F29200339: bbi-86364.c 
>
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?
>
> F29200339: bbi-86364.c 

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

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?

F29200339: bbi-86364.c 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c93e3c1c8ba: Take math-errno into account with 
#pragma float_control(precise,on) and (authored by zahiraam).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151834

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 fast float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f2
+// FAST: call fast float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1260,6 +1260,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2279,6 +2279,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5501,11 

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 556141.

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

https://reviews.llvm.org/D151834

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 fast float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f2
+// FAST: call fast float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1260,6 +1260,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2228,6 +2228,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5450,11 +5461,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if 

[PATCH] D151834: Include math-errno with fast-math

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor accepted this revision.
andrew.w.kaylor added a comment.

lgtm


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

@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.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 556057.

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

https://reviews.llvm.org/D151834

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 fast float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f2
+// FAST: call fast float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1260,6 +1260,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2228,6 +2228,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5450,11 +5461,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if 

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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!


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

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.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 556029.

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

https://reviews.llvm.org/D151834

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1259,6 +1259,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// Like the overload taking a `Function &`, but intended specifically
   /// for frontends that want to build on Clang's target-configuration logic.
   void addDefaultFunctionDefinitionAttributes(llvm::AttrBuilder );
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2228,6 +2228,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5450,11 +5461,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if 

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/UsersManual.rst:1727
 
-   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled 
(``-ffp-contract=on``).  This is the default behavior.
+   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled 
(``-ffp-contract=on``).  This is the default behavior. This value resets 
``-fmath-errno`` to its target-dependent default.
* ``strict`` Enables ``-frounding-math`` and 
``-ffp-exception-behavior=strict``, and disables contractions (FMA).  All of 
the ``-ffast-math`` enablements are disabled. Enables ``STDC FENV_ACCESS``: by 
default ``FENV_ACCESS`` is disabled. This option setting behaves as though 
``#pragma STDC FENV_ACCESS ON`` appeared at the top of the source file.

zahiraam wrote:
> aaron.ballman wrote:
> > Can you re-flow this to 80 columns?
> Did the same for the rest of the text too. Is that OK?
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).


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:4702
+optimizations are enabled in the section of code governed by the pragma.
+Effectively ``-ffast-math`` is enabled and ``-ffp-contract=fast``. This pragam
+disables ``-fmath-errno``.

aaron.ballman wrote:
> 
At least I was consistent :-)



Comment at: clang/docs/UsersManual.rst:1727
 
-   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled 
(``-ffp-contract=on``).  This is the default behavior.
+   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled 
(``-ffp-contract=on``).  This is the default behavior. This value resets 
``-fmath-errno`` to its target-dependent default.
* ``strict`` Enables ``-frounding-math`` and 
``-ffp-exception-behavior=strict``, and disables contractions (FMA).  All of 
the ``-ffast-math`` enablements are disabled. Enables ``STDC FENV_ACCESS``: by 
default ``FENV_ACCESS`` is disabled. This option setting behaves as though 
``#pragma STDC FENV_ACCESS ON`` appeared at the top of the source file.

aaron.ballman wrote:
> Can you re-flow this to 80 columns?
Did the same for the rest of the text too. Is that OK?


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 546055.
zahiraam marked an inline comment as done.

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

https://reviews.llvm.org/D151834

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if 

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:4698
 ``-ffast-math`` is disabled and ``-ffp-contract=on``
-(fused multiply add) is enabled.
+(fused multiply add) is enabled. This pragam enables ``-fmath-errno``.
+





Comment at: clang/docs/LanguageExtensions.rst:4702
+optimizations are enabled in the section of code governed by the pragma.
+Effectively ``-ffast-math`` is enabled and ``-ffp-contract=fast``. This pragam
+disables ``-fmath-errno``.





Comment at: clang/docs/UsersManual.rst:1727
 
-   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled 
(``-ffp-contract=on``).  This is the default behavior.
+   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled 
(``-ffp-contract=on``).  This is the default behavior. This value resets 
``-fmath-errno`` to its target-dependent default.
* ``strict`` Enables ``-frounding-math`` and 
``-ffp-exception-behavior=strict``, and disables contractions (FMA).  All of 
the ``-ffast-math`` enablements are disabled. Enables ``STDC FENV_ACCESS``: by 
default ``FENV_ACCESS`` is disabled. This option setting behaves as though 
``#pragma STDC FENV_ACCESS ON`` appeared at the top of the source file.

Can you re-flow this to 80 columns?


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 545666.
zahiraam marked an inline comment as done.

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

https://reviews.llvm.org/D151834

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if 

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2313-2315
+  // Math intrinsics are generated only when math-errno is disabled. Any pragma
+  // or attribute that affect math-errno, should prevent or allow math
+  // intrincs to be generated. Intrinsics are generated:




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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 544893.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if (FD->hasAttr())
   // All calls within a strictfp function are marked strictfp
   Attrs = 

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2321-2328
+  if ((FD->hasAttr() && !ErrnoOverriden && !OptNone) ||
   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+   (!ConstWithoutErrnoAndExceptions ||
+(!getLangOpts().MathErrno && !ErrnoOverriden && !OptNone) ||
+// If math-errno was enabled on command line but overriden to false
+// via '#pragma float_control(precise, off))', and optimizations are
+// enabled, generate intrinsics.

zahiraam wrote:
> aaron.ballman wrote:
> > zahiraam wrote:
> > > aaron.ballman wrote:
> > > > I think this has gotten sufficiently complex that it might be worth 
> > > > splitting the logic out a bit more:
> > > > ```
> > > > bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> > > > if (!Optimize) Optimize = FD->hasAttr() && !ErrnoOverriden 
> > > > && !OptNone;
> > > > ... and so on ...
> > > > ```
> > > > WDYT?
> > > I understand I need to split the logic, but I don't get your proposal 
> > > here. Do you mean?
> > > 
> > > 
> > > ```
> > > bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> > > if (!Optimize) Optimize = !getLangOpts().MathErrno && !ErrnoOverriden && 
> > > !OptNone;
> > > ... and so on ...
> > > ```
> > > 
> > 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.
> Changed the name of the variable you proposed. Is that OK?
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.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2321-2328
+  if ((FD->hasAttr() && !ErrnoOverriden && !OptNone) ||
   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+   (!ConstWithoutErrnoAndExceptions ||
+(!getLangOpts().MathErrno && !ErrnoOverriden && !OptNone) ||
+// If math-errno was enabled on command line but overriden to false
+// via '#pragma float_control(precise, off))', and optimizations are
+// enabled, generate intrinsics.

aaron.ballman wrote:
> zahiraam wrote:
> > aaron.ballman wrote:
> > > I think this has gotten sufficiently complex that it might be worth 
> > > splitting the logic out a bit more:
> > > ```
> > > bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> > > if (!Optimize) Optimize = FD->hasAttr() && !ErrnoOverriden && 
> > > !OptNone;
> > > ... and so on ...
> > > ```
> > > WDYT?
> > I understand I need to split the logic, but I don't get your proposal here. 
> > Do you mean?
> > 
> > 
> > ```
> > bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> > if (!Optimize) Optimize = !getLangOpts().MathErrno && !ErrnoOverriden && 
> > !OptNone;
> > ... and so on ...
> > ```
> > 
> 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.
Changed the name of the variable you proposed. Is that OK?


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 544745.
zahiraam marked 3 inline comments as done.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if (FD->hasAttr())
   // All calls within a strictfp function are 

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2321-2328
+  if ((FD->hasAttr() && !ErrnoOverriden && !OptNone) ||
   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+   (!ConstWithoutErrnoAndExceptions ||
+(!getLangOpts().MathErrno && !ErrnoOverriden && !OptNone) ||
+// If math-errno was enabled on command line but overriden to false
+// via '#pragma float_control(precise, off))', and optimizations are
+// enabled, generate intrinsics.

zahiraam wrote:
> aaron.ballman wrote:
> > I think this has gotten sufficiently complex that it might be worth 
> > splitting the logic out a bit more:
> > ```
> > bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> > if (!Optimize) Optimize = FD->hasAttr() && !ErrnoOverriden && 
> > !OptNone;
> > ... and so on ...
> > ```
> > WDYT?
> I understand I need to split the logic, but I don't get your proposal here. 
> Do you mean?
> 
> 
> ```
> bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> if (!Optimize) Optimize = !getLangOpts().MathErrno && !ErrnoOverriden && 
> !OptNone;
> ... and so on ...
> ```
> 
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.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 544084.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if (FD->hasAttr())
   // All calls within a strictfp function are marked strictfp
   Attrs = 

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2321-2328
+  if ((FD->hasAttr() && !ErrnoOverriden && !OptNone) ||
   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+   (!ConstWithoutErrnoAndExceptions ||
+(!getLangOpts().MathErrno && !ErrnoOverriden && !OptNone) ||
+// If math-errno was enabled on command line but overriden to false
+// via '#pragma float_control(precise, off))', and optimizations are
+// enabled, generate intrinsics.

aaron.ballman wrote:
> I think this has gotten sufficiently complex that it might be worth splitting 
> the logic out a bit more:
> ```
> bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
> if (!Optimize) Optimize = FD->hasAttr() && !ErrnoOverriden && 
> !OptNone;
> ... and so on ...
> ```
> WDYT?
I understand I need to split the logic, but I don't get your proposal here. Do 
you mean?


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



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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam marked 2 inline comments as done.
zahiraam added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.h:857
 setAllowApproxFuncOverride(!Value);
+setMathErrnoOverride(Value);
 if (Value)

aaron.ballman wrote:
> zahiraam wrote:
> > aaron.ballman wrote:
> > > Everything else does `!Value`; is it intentional that you're using 
> > > `Value` instead?
> > Yes. I want to set the value of math-errno to the value of the pragma in 
> > the source.
> 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?
Correct.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/FPOptions.def:29
 OPTION(Float16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)

zahiraam wrote:
> aaron.ballman wrote:
> > Shouldn't this one be `Float16ExcessPrecision`? (Are we missing test 
> > coverage that would have caught that?)
> oops! the test would be for the BFloat16ExcessPrecision, right?
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.



Comment at: clang/include/clang/Basic/LangOptions.h:857
 setAllowApproxFuncOverride(!Value);
+setMathErrnoOverride(Value);
 if (Value)

zahiraam wrote:
> aaron.ballman wrote:
> > Everything else does `!Value`; is it intentional that you're using `Value` 
> > instead?
> Yes. I want to set the value of math-errno to the value of the pragma in the 
> source.
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?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2297
+
+  // True if we are compiling at -O2 and errno has been disabled
+  // using the '#pragma float_control(precise, off)', and attribute opt-none

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


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/include/clang/Basic/FPOptions.def:29
 OPTION(Float16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)

aaron.ballman wrote:
> Shouldn't this one be `Float16ExcessPrecision`? (Are we missing test coverage 
> that would have caught that?)
oops! the test would be for the BFloat16ExcessPrecision, right?



Comment at: clang/include/clang/Basic/LangOptions.h:857
 setAllowApproxFuncOverride(!Value);
+setMathErrnoOverride(Value);
 if (Value)

aaron.ballman wrote:
> Everything else does `!Value`; is it intentional that you're using `Value` 
> instead?
Yes. I want to set the value of math-errno to the value of the pragma in the 
source.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: efriedma.
aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/FPOptions.def:29
 OPTION(Float16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)

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



Comment at: clang/include/clang/Basic/LangOptions.h:857
 setAllowApproxFuncOverride(!Value);
+setMathErrnoOverride(Value);
 if (Value)

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



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2321-2328
+  if ((FD->hasAttr() && !ErrnoOverriden && !OptNone) ||
   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+   (!ConstWithoutErrnoAndExceptions ||
+(!getLangOpts().MathErrno && !ErrnoOverriden && !OptNone) ||
+// If math-errno was enabled on command line but overriden to false
+// via '#pragma float_control(precise, off))', and optimizations are
+// enabled, generate intrinsics.

I think this has gotten sufficiently complex that it might be worth splitting 
the logic out a bit more:
```
bool Optimize = FD->hasAttr() && !ErrnoOverriden && !OptNone;
if (!Optimize) Optimize = FD->hasAttr() && !ErrnoOverriden && 
!OptNone;
... and so on ...
```
WDYT?


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

Review anyone please? Thanks.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/test/CodeGen/math-errno.c:28
+// 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]+]]
+//

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



Comment at: clang/test/CodeGen/math-errno.c:40
+// CHECK-LABEL: define dso_local float @f2
+// CHECK: tail call float @llvm.sqrt.f32(float {{.*}})
+

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



Comment at: clang/test/CodeGen/math-errno.c:2
+// Tests that at -O2 math-errno is taken into account. Same than MSVC.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration -fmath-errno \
+// RUN: -O2 -emit-llvm -o - %s \

andrew.w.kaylor wrote:
> Isn't math-errno true by default when fast-math isn't used?
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. 


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 542141.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,18 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if (FD->hasAttr())
   // All calls within a strictfp function are marked strictfp
   Attrs = 

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 539961.
zahiraam marked 6 inline comments as done and an inline comment as not done.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,64 @@
+// -O2
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -fmath-errno -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: -fmath-errno -ffp-contract=on -fno-rounding-math -O0 \
+// RUN: -emit-llvm -o - %s | 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: 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 float @llvm.sqrt.f32(float {{.*}})
+
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: call float @sqrtf(float {{.*}}) #[[ATTR4_NOOPT:[0-9]+]]
+
+__attribute__((optnone))
+float f3(float x) {
+  x = sqrtf(x);
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @sqrtf(float noundef {{.*}})
+
+// 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 @sqrtf(float noundef %0) #[[ATTR4_NOOPT:[0-9]+]]
+
+// CHECK: [[ATTR4_O2]] = { nounwind }
+// FAST: [[ATTR3_FAST]] =  { nounwind willreturn memory(none) }
+// NOOPT: [[ATTR4_NOOPT]] = { nounwind }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1252,6 +1252,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2247,6 +2247,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5442,11 +5453,17 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if (FD->hasAttr())
   // All calls 

[PATCH] D151834: Include math-errno with fast-math

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2249
 
+  // MSVC takes into account math-errno when compiling at -O2 level in
+  // the presence of a '#pragma float_control precise(on/off)'.

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.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2263
+  }
+  bool ErrnoWithO2OrFastMath =
+  MathErrnoOverrride && (CGM.getCodeGenOpts().OptimizationLevel == 2 ||

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?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2266
+ CGM.getLangOpts().FastMath);
+  bool OptNoneWithFastMath =
+  CurFuncDecl->hasAttr() && CGM.getLangOpts().FastMath;

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



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2284
+
+  if ((FD->hasAttr() && EmitIntrinsic) || EmitIntrinsic ||
+   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&

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.



Comment at: clang/test/CodeGen/math-errno.c:28
+// 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]+]]
+//

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?



Comment at: clang/test/CodeGen/math-errno.c:40
+// CHECK-LABEL: define dso_local float @f2
+// CHECK: tail call float @llvm.sqrt.f32(float {{.*}})
+

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.



Comment at: clang/test/CodeGen/math-errno.c:43
+// 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]]
+

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



Comment at: clang/test/CodeGen/math-errno.c:46
+// NOOPT-LABEL: define dso_local float @f2
+// NOOPT: tail call float @llvm.sqrt.f32(float {{.*}})
+

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.



Comment at: clang/test/CodeGen/math-errno.c:56
+// CHECK-LABEL: define dso_local float @f3
+// CHECK: call float @llvm.sqrt.f32(float {{.*}})
+

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


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/include/clang/Basic/FPOptions.def:30
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)
 #undef OPTION

andrew.w.kaylor wrote:
> Does this mean MathErrno is tracked in both LangOpts and FPOptions?
Yes. So that we can check that if it's overridden by a pragma or an attribute. 
I don't see another way.



Comment at: clang/test/CodeGen/math-errno.c:33
+
+__attribute__((optnone))
+float f4(float x) { 

andrew.w.kaylor wrote:
> Can you add a runline with -O0. That should prevent all instances of the 
> intrinsics, right?
Yes.



Comment at: clang/test/CodeGen/math-errno.c:49
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f4(float noundef 
nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float 
@sqrtf(float noundef nofpclass(nan inf) %0) #[[ATTR0:[0-9]+]]
+

andrew.w.kaylor wrote:
> 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.
Added all the commands that are triggered by fast-math in the RUN line command. 


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 530040.
zahiraam marked 5 inline comments as done.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,66 @@
+// -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) }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1254,6 +1254,12 @@
   llvm::AttributeList , unsigned ,
   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 );
+
   /// 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.
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2216,6 +2216,17 @@
   return Mask;
 }
 
+void CodeGenModule::AdjustMemoryAttribute(StringRef Name,
+  CGCalleeInfo CalleeInfo,
+  llvm::AttributeList ) {
+  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:
@@ -5463,11 +5474,17 @@
  /*AttrOnCallSite=*/true,
  /*IsThunk=*/false);
 
-  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl)) {
 if 

[PATCH] D151834: Include math-errno with fast-math

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/include/clang/Basic/FPOptions.def:30
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, 
FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)
 #undef OPTION

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



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2248
   FD->hasAttr() ? 0 : BuiltinID;
+  bool MathErrnoOverrride = false;
+  if (E->hasStoredFPFeatures()) {

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.



Comment at: clang/lib/CodeGen/CGCall.cpp:2384
 
+  if (HasOptnone && !getLangOpts().MathErrno)
+OptNone = false;

I don't understand what this is doing.



Comment at: clang/lib/CodeGen/CodeGenModule.h:611
   void clear();
+  bool OptNone = true;
 

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



Comment at: clang/test/CodeGen/math-errno.c:2
+// Tests that at -O2 math-errno is taken into account. Same than MSVC.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration -fmath-errno \
+// RUN: -O2 -emit-llvm -o - %s \

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



Comment at: clang/test/CodeGen/math-errno.c:33
+
+__attribute__((optnone))
+float f4(float x) { 

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



Comment at: clang/test/CodeGen/math-errno.c:49
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f4(float noundef 
nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float 
@sqrtf(float noundef nofpclass(nan inf) %0) #[[ATTR0:[0-9]+]]
+

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.


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

https://reviews.llvm.org/D151834

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 527950.

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

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,52 @@
+// Tests that at -O2 math-errno is taken into account. Same than MSVC.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration -fmath-errno \
+// RUN: -O2 -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+
+// Tests that with -ffast-math math-errno is taken into account with
+// float_control(precise,on) and optnone.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -ffast-math -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefix=FAST
+
+float f1(float x) { 
+#pragma float_control(precise,on)
+return sqrtf(x);
+}
+
+float f2(float x) { 
+#pragma float_control(precise,on)
+  {
+float y = sqrtf(x);
+return y;
+  }
+}
+
+float f3(float x) { 
+#pragma float_control(precise,off)
+  {
+float y = sqrtf(x);
+  return y;
+  }
+}
+
+__attribute__((optnone))
+float f4(float x) { 
+  x = sqrtf(x); 
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f2(float noundef {{.*}})
+// CHECK:  tail call float @sqrtf(float noundef {{.*}})
+
+// CHECK-LABEL: define dso_local float @f3(float noundef {{.*}})
+// CHECK: call float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f1(float noundef nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) {{.*}}) #[[ATTR0:[0-9]+]]
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f4(float noundef nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) %0) #[[ATTR0:[0-9]+]]
+
+// FAST: attributes #[[ATTR0]] = { {{.*}} memory(none) }
+
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -608,6 +608,7 @@
   ~CodeGenModule();
 
   void clear();
+  bool OptNone = true;
 
   /// Finalize LLVM code generation.
   void Release();
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2381,6 +2381,9 @@
   // Collect function IR attributes based on global settiings.
   getDefaultFunctionAttributes(Name, HasOptnone, AttrOnCallSite, FuncAttrs);
 
+  if (HasOptnone && !getLangOpts().MathErrno)
+OptNone = false;
+
   // Override some default IR attributes based on declaration-specific
   // information.
   if (TargetDecl) {
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2245,6 +2245,13 @@
   // likely to get lowered to the renamed library functions.
   const unsigned BuiltinIDIfNoAsmLabel =
   FD->hasAttr() ? 0 : BuiltinID;
+  bool MathErrnoOverrride = false;
+  if (E->hasStoredFPFeatures()) {
+FPOptionsOverride OP = E->getFPFeatures();
+if (OP.hasMathErrnoOverride())
+  MathErrnoOverrride = OP.getMathErrnoOverride();
+  }
+  bool EmitIntrinsic = !MathErrnoOverrride && CGM.OptNone;
 
   // There are LLVM math intrinsics/instructions corresponding to math library
   // functions except the LLVM op will never set errno while the math library
@@ -2257,9 +2264,12 @@
   getContext().BuiltinInfo.isConstWithoutErrnoAndExceptions(BuiltinID);
   bool ConstWithoutExceptions =
   getContext().BuiltinInfo.isConstWithoutExceptions(BuiltinID);
-  if (FD->hasAttr() ||
-  ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+
+  if ((FD->hasAttr() && EmitIntrinsic) ||
+  (EmitIntrinsic ||
+   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
+ (!ConstWithoutErrnoAndExceptions ||
+ (!getLangOpts().MathErrno && EmitIntrinsic) {
 switch (BuiltinIDIfNoAsmLabel) {
 case Builtin::BIceil:
 case Builtin::BIceilf:
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -850,6 +850,7 @@
 setNoSignedZeroOverride(!Value);
 setAllowReciprocalOverride(!Value);
 setAllowApproxFuncOverride(!Value);
+setMathErrnoOverride(Value);
 if (Value)
   /* Precise mode implies fp_contract=on and disables ffast-math */
   

[PATCH] D151834: Include math-errno with fast-math

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam created this revision.
zahiraam added reviewers: andrew.w.kaylor, jcranmer-intel.
Herald added a project: All.
zahiraam requested review of this revision.
Herald added a project: clang.

MSVC includes math-errno with #pragma float_control(precise,on) at O2 
. 
At fast-math math-errno should also be included with optnone and the pragma.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151834

Files:
  clang/include/clang/Basic/FPOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/math-errno.c

Index: clang/test/CodeGen/math-errno.c
===
--- /dev/null
+++ clang/test/CodeGen/math-errno.c
@@ -0,0 +1,52 @@
+// Tests that at -O2 math-errno is taken into account. Same than MSVC.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration -fmath-errno \
+// RUN: -O2 -emit-llvm -o - %s \
+// RUN: | FileCheck %s
+
+// Tests that with -ffast-math math-errno is taken into account with
+// float_control(precise,on) and optnone.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration  \
+// RUN: -ffast-math -emit-llvm -o - %s \
+// RUN: | FileCheck %s -check-prefix=FAST
+
+float f1(float x) { 
+#pragma float_control(precise,on)
+return sqrtf(x);
+}
+
+float f2(float x) { 
+#pragma float_control(precise,on)
+  {
+float y = sqrtf(x);
+return y;
+  }
+}
+
+float f3(float x) { 
+#pragma float_control(precise,off)
+  {
+float y = sqrtf(x);
+  return y;
+  }
+}
+
+__attribute__((optnone))
+float f4(float x) { 
+  x = sqrtf(x); 
+  return x;
+}
+
+// CHECK-LABEL: define dso_local float @f2(float noundef {{.*}})
+// CHECK:  tail call float @sqrtf(float noundef {{.*}})
+
+// CHECK-LABEL: define dso_local float @f3(float noundef {{.*}})
+// CHECK: call float @llvm.sqrt.f32(float {{.*}})
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f1(float noundef nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) {{.*}}) #[[ATTR0:[0-9]+]]
+
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f4(float noundef nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) %0) #[[ATTR0:[0-9]+]]
+
+// FAST: attributes #[[ATTR0]] = { {{.*}} memory(none) }
+
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -608,6 +608,7 @@
   ~CodeGenModule();
 
   void clear();
+  bool OptNone = true;
 
   /// Finalize LLVM code generation.
   void Release();
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2381,6 +2381,9 @@
   // Collect function IR attributes based on global settiings.
   getDefaultFunctionAttributes(Name, HasOptnone, AttrOnCallSite, FuncAttrs);
 
+  if (HasOptnone && !getLangOpts().MathErrno)
+OptNone = false;
+
   // Override some default IR attributes based on declaration-specific
   // information.
   if (TargetDecl) {
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2229,6 +2229,13 @@
   // likely to get lowered to the renamed library functions.
   const unsigned BuiltinIDIfNoAsmLabel =
   FD->hasAttr() ? 0 : BuiltinID;
+  bool MathErrnoOverrride = false;
+  if (E->hasStoredFPFeatures()) {
+FPOptionsOverride OP = E->getFPFeatures();
+if (OP.hasMathErrnoOverride())
+  MathErrnoOverrride = OP.getMathErrnoOverride();
+  }
+  bool EmitIntrinsic = !MathErrnoOverrride && CGM.OptNone;
 
   // There are LLVM math intrinsics/instructions corresponding to math library
   // functions except the LLVM op will never set errno while the math library
@@ -2241,9 +2248,12 @@
   getContext().BuiltinInfo.isConstWithoutErrnoAndExceptions(BuiltinID);
   bool ConstWithoutExceptions =
   getContext().BuiltinInfo.isConstWithoutExceptions(BuiltinID);
-  if (FD->hasAttr() ||
-  ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
-   (!ConstWithoutErrnoAndExceptions || (!getLangOpts().MathErrno {
+
+  if ((FD->hasAttr() && EmitIntrinsic) ||
+  (EmitIntrinsic ||
+   ((ConstWithoutErrnoAndExceptions || ConstWithoutExceptions) &&
+ (!ConstWithoutErrnoAndExceptions ||
+ (!getLangOpts().MathErrno && EmitIntrinsic) {
 switch (BuiltinIDIfNoAsmLabel) {
 case Builtin::BIceil:
 case Builtin::BIceilf:
Index: clang/include/clang/Basic/LangOptions.h