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
