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
