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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
