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