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
