[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm closed this revision.
arsenm added a comment.

42d4c85ca83f25f993444fb5bbaa58525f724991 



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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

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

self accept after latest comments


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In D139629#4481408 , @rjmccall wrote:

> In D139629#4481030 , @zahiraam 
> wrote:
>
>> According to this comment https://reviews.llvm.org/D87528#2342132, it looks 
>> like @rjmccall had proposed the addition of the attribute. @rjmccall Do you 
>> recall why it was added?
>
> My suggestion was that we track the pragma in the Clang AST with an attribute 
> instead of a bit on the Decl.  I have no idea why IR generation is adding 
> both a string attribute and the builtin attribute to the IR function, but if 
> doesn't have any obvious effect, I suggest just removing it.

Thanks for your response. I second removing it then.


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D139629#4481030 , @zahiraam wrote:

> According to this comment https://reviews.llvm.org/D87528#2342132, it looks 
> like @rjmccall had proposed the addition of the attribute. @rjmccall Do you 
> recall why it was added?

My suggestion was that we track the pragma in the Clang AST with an attribute 
instead of a bit on the Decl.  I have no idea why IR generation is adding both 
a string attribute and the builtin attribute to the IR function, but if doesn't 
have any obvious effect, I suggest just removing it.


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a subscriber: rjmccall.
zahiraam added a comment.

According to this comment https://reviews.llvm.org/D87528#2342132, it looks 
like @rjmccall had proposed the addition of the attribute. @rjmccall Do you 
recall why it was added?


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

ping


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

ping


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

arsenm wrote:
> kpn wrote:
> > arsenm wrote:
> > > andrew.w.kaylor wrote:
> > > > arsenm wrote:
> > > > > zahiraam wrote:
> > > > > > I think it would better to fix this function instead of removing it 
> > > > > > entirely? The issue here is that there is the "strictfp" attribute 
> > > > > > and the llvm::Attribute::StrictFP. We could replace  
> > > > > > FuncAttrs.addAttribute("strictfp"); with
> > > > > >  FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
> > > > > > This function ensures that the function attribute is set when the 
> > > > > > FunctionDecl attribute is set. I am concerned that when it's 
> > > > > > removed, we will wind up with cases where the function attribute is 
> > > > > > missing! The only place where this function attribute is in 
> > > > > > CodeGenFunction::StartFunction. Is that enough? @andrew.w.kaylor 
> > > > > > Can you please weigh in on this?
> > > > > I currently don't have evidence that making this use the correct 
> > > > > attribute would fix anything. If something was depending on this 
> > > > > emission in this context, it's already broken
> > > > It may be that anything depending on this is already broken, but the 
> > > > code was written for a reason, even if it was always broken. I'm not 
> > > > sure I understand what that reason was, and unfortunately the person 
> > > > who wrote the code (@mibintc) is no longer actively contributing to 
> > > > LLVM. It was added here: https://reviews.llvm.org/D87528
> > > > 
> > > > It does seem like the llvm::Attribute::StrictFP is being added any time 
> > > > the string attribute is added, but they're coming from different 
> > > > places. The proper attribute seems to be coming from 
> > > > CodeGenFunction::StartFunction() which is effectively copying it from 
> > > > the function declaration. It's not clear to me whether there are 
> > > > circumstances where we get to setLLVMFunctionFEnvAttributes() through 
> > > > EmitGlobalFunctionDefinition() without ever having gone through 
> > > > CodeGenFunction::StartFunction(). It looks like maybe there are 
> > > > multiversioning cases that do that, but I couldn't come up with an 
> > > > example that does. @erichkeane wrote a lot of the multi-versioning 
> > > > code, so he might know more, but he's on vacation through the end of 
> > > > the month.
> > > > 
> > > > Eliminating this extra string attribute seems obviously good. In this 
> > > > particular location, though, I'd be inclined to set the enumerated 
> > > > attribute here, even though that might be redundant in most cases. On 
> > > > the other hand, if one of our front end experts can look at this code 
> > > > and say definitively that it's //always// redundant, I'd be fine with 
> > > > this code being deleted.
> > > I think code that can be deleted that doesn't manifest in test failures 
> > > should be immediately deleted. We shouldn't leave things around just in 
> > > case 
> > The Verifier changes that would detect the error and fail tests never made 
> > it into the tree. The lack of failures therefore tells us nothing in this 
> > case here.
> The verifier never would have checked the string form
Ah, true statement.


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

kpn wrote:
> arsenm wrote:
> > andrew.w.kaylor wrote:
> > > arsenm wrote:
> > > > zahiraam wrote:
> > > > > I think it would better to fix this function instead of removing it 
> > > > > entirely? The issue here is that there is the "strictfp" attribute 
> > > > > and the llvm::Attribute::StrictFP. We could replace  
> > > > > FuncAttrs.addAttribute("strictfp"); with
> > > > >  FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
> > > > > This function ensures that the function attribute is set when the 
> > > > > FunctionDecl attribute is set. I am concerned that when it's removed, 
> > > > > we will wind up with cases where the function attribute is missing! 
> > > > > The only place where this function attribute is in 
> > > > > CodeGenFunction::StartFunction. Is that enough? @andrew.w.kaylor Can 
> > > > > you please weigh in on this?
> > > > I currently don't have evidence that making this use the correct 
> > > > attribute would fix anything. If something was depending on this 
> > > > emission in this context, it's already broken
> > > It may be that anything depending on this is already broken, but the code 
> > > was written for a reason, even if it was always broken. I'm not sure I 
> > > understand what that reason was, and unfortunately the person who wrote 
> > > the code (@mibintc) is no longer actively contributing to LLVM. It was 
> > > added here: https://reviews.llvm.org/D87528
> > > 
> > > It does seem like the llvm::Attribute::StrictFP is being added any time 
> > > the string attribute is added, but they're coming from different places. 
> > > The proper attribute seems to be coming from 
> > > CodeGenFunction::StartFunction() which is effectively copying it from the 
> > > function declaration. It's not clear to me whether there are 
> > > circumstances where we get to setLLVMFunctionFEnvAttributes() through 
> > > EmitGlobalFunctionDefinition() without ever having gone through 
> > > CodeGenFunction::StartFunction(). It looks like maybe there are 
> > > multiversioning cases that do that, but I couldn't come up with an 
> > > example that does. @erichkeane wrote a lot of the multi-versioning code, 
> > > so he might know more, but he's on vacation through the end of the month.
> > > 
> > > Eliminating this extra string attribute seems obviously good. In this 
> > > particular location, though, I'd be inclined to set the enumerated 
> > > attribute here, even though that might be redundant in most cases. On the 
> > > other hand, if one of our front end experts can look at this code and say 
> > > definitively that it's //always// redundant, I'd be fine with this code 
> > > being deleted.
> > I think code that can be deleted that doesn't manifest in test failures 
> > should be immediately deleted. We shouldn't leave things around just in 
> > case 
> The Verifier changes that would detect the error and fail tests never made it 
> into the tree. The lack of failures therefore tells us nothing in this case 
> here.
The verifier never would have checked the string form


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

arsenm wrote:
> andrew.w.kaylor wrote:
> > arsenm wrote:
> > > zahiraam wrote:
> > > > I think it would better to fix this function instead of removing it 
> > > > entirely? The issue here is that there is the "strictfp" attribute and 
> > > > the llvm::Attribute::StrictFP. We could replace  
> > > > FuncAttrs.addAttribute("strictfp"); with
> > > >  FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
> > > > This function ensures that the function attribute is set when the 
> > > > FunctionDecl attribute is set. I am concerned that when it's removed, 
> > > > we will wind up with cases where the function attribute is missing! The 
> > > > only place where this function attribute is in 
> > > > CodeGenFunction::StartFunction. Is that enough? @andrew.w.kaylor Can 
> > > > you please weigh in on this?
> > > I currently don't have evidence that making this use the correct 
> > > attribute would fix anything. If something was depending on this emission 
> > > in this context, it's already broken
> > It may be that anything depending on this is already broken, but the code 
> > was written for a reason, even if it was always broken. I'm not sure I 
> > understand what that reason was, and unfortunately the person who wrote the 
> > code (@mibintc) is no longer actively contributing to LLVM. It was added 
> > here: https://reviews.llvm.org/D87528
> > 
> > It does seem like the llvm::Attribute::StrictFP is being added any time the 
> > string attribute is added, but they're coming from different places. The 
> > proper attribute seems to be coming from CodeGenFunction::StartFunction() 
> > which is effectively copying it from the function declaration. It's not 
> > clear to me whether there are circumstances where we get to 
> > setLLVMFunctionFEnvAttributes() through EmitGlobalFunctionDefinition() 
> > without ever having gone through CodeGenFunction::StartFunction(). It looks 
> > like maybe there are multiversioning cases that do that, but I couldn't 
> > come up with an example that does. @erichkeane wrote a lot of the 
> > multi-versioning code, so he might know more, but he's on vacation through 
> > the end of the month.
> > 
> > Eliminating this extra string attribute seems obviously good. In this 
> > particular location, though, I'd be inclined to set the enumerated 
> > attribute here, even though that might be redundant in most cases. On the 
> > other hand, if one of our front end experts can look at this code and say 
> > definitively that it's //always// redundant, I'd be fine with this code 
> > being deleted.
> I think code that can be deleted that doesn't manifest in test failures 
> should be immediately deleted. We shouldn't leave things around just in case 
The Verifier changes that would detect the error and fail tests never made it 
into the tree. The lack of failures therefore tells us nothing in this case 
here.


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

andrew.w.kaylor wrote:
> arsenm wrote:
> > zahiraam wrote:
> > > I think it would better to fix this function instead of removing it 
> > > entirely? The issue here is that there is the "strictfp" attribute and 
> > > the llvm::Attribute::StrictFP. We could replace  
> > > FuncAttrs.addAttribute("strictfp"); with
> > >  FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
> > > This function ensures that the function attribute is set when the 
> > > FunctionDecl attribute is set. I am concerned that when it's removed, we 
> > > will wind up with cases where the function attribute is missing! The only 
> > > place where this function attribute is in CodeGenFunction::StartFunction. 
> > > Is that enough? @andrew.w.kaylor Can you please weigh in on this?
> > I currently don't have evidence that making this use the correct attribute 
> > would fix anything. If something was depending on this emission in this 
> > context, it's already broken
> It may be that anything depending on this is already broken, but the code was 
> written for a reason, even if it was always broken. I'm not sure I understand 
> what that reason was, and unfortunately the person who wrote the code 
> (@mibintc) is no longer actively contributing to LLVM. It was added here: 
> https://reviews.llvm.org/D87528
> 
> It does seem like the llvm::Attribute::StrictFP is being added any time the 
> string attribute is added, but they're coming from different places. The 
> proper attribute seems to be coming from CodeGenFunction::StartFunction() 
> which is effectively copying it from the function declaration. It's not clear 
> to me whether there are circumstances where we get to 
> setLLVMFunctionFEnvAttributes() through EmitGlobalFunctionDefinition() 
> without ever having gone through CodeGenFunction::StartFunction(). It looks 
> like maybe there are multiversioning cases that do that, but I couldn't come 
> up with an example that does. @erichkeane wrote a lot of the multi-versioning 
> code, so he might know more, but he's on vacation through the end of the 
> month.
> 
> Eliminating this extra string attribute seems obviously good. In this 
> particular location, though, I'd be inclined to set the enumerated attribute 
> here, even though that might be redundant in most cases. On the other hand, 
> if one of our front end experts can look at this code and say definitively 
> that it's //always// redundant, I'd be fine with this code being deleted.
I think code that can be deleted that doesn't manifest in test failures should 
be immediately deleted. We shouldn't leave things around just in case 


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a subscriber: erichkeane.
andrew.w.kaylor added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

arsenm wrote:
> zahiraam wrote:
> > I think it would better to fix this function instead of removing it 
> > entirely? The issue here is that there is the "strictfp" attribute and the 
> > llvm::Attribute::StrictFP. We could replace  
> > FuncAttrs.addAttribute("strictfp"); with
> >  FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
> > This function ensures that the function attribute is set when the 
> > FunctionDecl attribute is set. I am concerned that when it's removed, we 
> > will wind up with cases where the function attribute is missing! The only 
> > place where this function attribute is in CodeGenFunction::StartFunction. 
> > Is that enough? @andrew.w.kaylor Can you please weigh in on this?
> I currently don't have evidence that making this use the correct attribute 
> would fix anything. If something was depending on this emission in this 
> context, it's already broken
It may be that anything depending on this is already broken, but the code was 
written for a reason, even if it was always broken. I'm not sure I understand 
what that reason was, and unfortunately the person who wrote the code 
(@mibintc) is no longer actively contributing to LLVM. It was added here: 
https://reviews.llvm.org/D87528

It does seem like the llvm::Attribute::StrictFP is being added any time the 
string attribute is added, but they're coming from different places. The proper 
attribute seems to be coming from CodeGenFunction::StartFunction() which is 
effectively copying it from the function declaration. It's not clear to me 
whether there are circumstances where we get to setLLVMFunctionFEnvAttributes() 
through EmitGlobalFunctionDefinition() without ever having gone through 
CodeGenFunction::StartFunction(). It looks like maybe there are multiversioning 
cases that do that, but I couldn't come up with an example that does. 
@erichkeane wrote a lot of the multi-versioning code, so he might know more, 
but he's on vacation through the end of the month.

Eliminating this extra string attribute seems obviously good. In this 
particular location, though, I'd be inclined to set the enumerated attribute 
here, even though that might be redundant in most cases. On the other hand, if 
one of our front end experts can look at this code and say definitively that 
it's //always// redundant, I'd be fine with this code being deleted.


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

zahiraam wrote:
> I think it would better to fix this function instead of removing it entirely? 
> The issue here is that there is the "strictfp" attribute and the 
> llvm::Attribute::StrictFP. We could replace  
> FuncAttrs.addAttribute("strictfp"); with
>  FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
> This function ensures that the function attribute is set when the 
> FunctionDecl attribute is set. I am concerned that when it's removed, we will 
> wind up with cases where the function attribute is missing! The only place 
> where this function attribute is in CodeGenFunction::StartFunction. Is that 
> enough? @andrew.w.kaylor Can you please weigh in on this?
I currently don't have evidence that making this use the correct attribute 
would fix anything. If something was depending on this emission in this 
context, it's already broken


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2135
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);

I think it would better to fix this function instead of removing it entirely? 
The issue here is that there is the "strictfp" attribute and the 
llvm::Attribute::StrictFP. We could replace  
FuncAttrs.addAttribute("strictfp"); with
 FuncAttrs.addAttribute(llvm::Attribute::StrictFP); 
This function ensures that the function attribute is set when the FunctionDecl 
attribute is set. I am concerned that when it's removed, we will wind up with 
cases where the function attribute is missing! The only place where this 
function attribute is in CodeGenFunction::StartFunction. Is that enough? 
@andrew.w.kaylor Can you please weigh in on this?


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D139629#3982052 , @arsenm wrote:

> In D139629#3981991 , @zahiraam 
> wrote:
>
>> Can you please be more verbose in your summary? This change is related to 
>> the use of -ffp-exception-behavior=strict right? 
>> This attribute is set here 
>> https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.cpp#L987
>>  but under a condition. Is that condition always satisfied when the 
>> attribute needs to be set?
>
> I don't know if there are other cases that misses; if so it's broken anyway. 
> Nothing reads this string formed version

It's coming from the FunctionDecl and the other case is broader. Only question 
would be of reachability


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D139629#3981991 , @zahiraam wrote:

> Can you please be more verbose in your summary? This change is related to the 
> use of -ffp-exception-behavior=strict right? 
> This attribute is set here 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.cpp#L987
>  but under a condition. Is that condition always satisfied when the attribute 
> needs to be set?

I don't know if there are other cases that misses; if so it's broken anyway. 
Nothing reads this string formed version


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

Can you please be more verbose in your summary? This change is related to the 
use of -ffp-exception-behavior=strict right? 
This attribute is set here 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CodeGenFunction.cpp#L987
 but under a condition. Is that condition always satisfied when the attribute 
needs to be set?


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

It looks like clang does have at least one test that checks for the strictfp 
attribute on function definitions. We also have a number that test for the 
strictfp attribute on function calls. So I think our test coverage is in good 
shape.

LGTM.


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

https://reviews.llvm.org/D139629

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

[PATCH] D139629: clang: Stop emitting "strictfp"

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: mibintc, john.brawn, kpn, jcranmer-intel, sepavloff, 
zahiraam.
Herald added a project: All.
arsenm requested review of this revision.
Herald added a subscriber: wdng.

The attribute is a proper enum attribute, strictfp. We were getting
strictfp and "strictfp" set on every function with
-fexperimental-strict-floating-point.


https://reviews.llvm.org/D139629

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/fp-floatcontrol-stack.cpp


Index: clang/test/CodeGen/fp-floatcontrol-stack.cpp
===
--- clang/test/CodeGen/fp-floatcontrol-stack.cpp
+++ clang/test/CodeGen/fp-floatcontrol-stack.cpp
@@ -284,3 +284,6 @@
 // CHECK-FAST: Function Attrs: noinline nounwind{{$$}}
 // CHECK-NOHONOR: Function Attrs: noinline nounwind{{$$}}
 // CHECK-LABEL: define{{.*}} @_GLOBAL__sub_I_fp_floatcontrol_stack
+
+// CHECK-DEBSTRICT: {{[ ]}}strictfp{{[ ]}}
+// CHECK-DEBSTRICT-NOT: "strictfp"
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2128,15 +2128,6 @@
   }
 }
 
-void CodeGenModule::setLLVMFunctionFEnvAttributes(const FunctionDecl *D,
-  llvm::Function *F) {
-  if (D->hasAttr()) {
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);
-  }
-}
-
 void CodeGenModule::SetCommonAttributes(GlobalDecl GD, llvm::GlobalValue *GV) {
   const Decl *D = GD.getDecl();
   if (isa_and_nonnull(D))
@@ -5330,9 +5321,6 @@
 
   maybeSetTrivialComdat(*D, *Fn);
 
-  // Set CodeGen attributes that represent floating point environment.
-  setLLVMFunctionFEnvAttributes(D, Fn);
-
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);
 
   setNonAliasAttributes(GD, Fn);


Index: clang/test/CodeGen/fp-floatcontrol-stack.cpp
===
--- clang/test/CodeGen/fp-floatcontrol-stack.cpp
+++ clang/test/CodeGen/fp-floatcontrol-stack.cpp
@@ -284,3 +284,6 @@
 // CHECK-FAST: Function Attrs: noinline nounwind{{$$}}
 // CHECK-NOHONOR: Function Attrs: noinline nounwind{{$$}}
 // CHECK-LABEL: define{{.*}} @_GLOBAL__sub_I_fp_floatcontrol_stack
+
+// CHECK-DEBSTRICT: {{[ ]}}strictfp{{[ ]}}
+// CHECK-DEBSTRICT-NOT: "strictfp"
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2128,15 +2128,6 @@
   }
 }
 
-void CodeGenModule::setLLVMFunctionFEnvAttributes(const FunctionDecl *D,
-  llvm::Function *F) {
-  if (D->hasAttr()) {
-llvm::AttrBuilder FuncAttrs(F->getContext());
-FuncAttrs.addAttribute("strictfp");
-F->addFnAttrs(FuncAttrs);
-  }
-}
-
 void CodeGenModule::SetCommonAttributes(GlobalDecl GD, llvm::GlobalValue *GV) {
   const Decl *D = GD.getDecl();
   if (isa_and_nonnull(D))
@@ -5330,9 +5321,6 @@
 
   maybeSetTrivialComdat(*D, *Fn);
 
-  // Set CodeGen attributes that represent floating point environment.
-  setLLVMFunctionFEnvAttributes(D, Fn);
-
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);
 
   setNonAliasAttributes(GD, Fn);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits