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
