[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