[PATCH] D70451: [FPEnv] IRBuilder should not put strictfp on function definitions automatically

[Melanie Blower via Phabricator via llvm-commits]

mibintc added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:262
     Function *F = BB->getParent();
+    assert(F && "Must have a function to set any function attributes!");
     if (!F->hasFnAttribute(Attribute::StrictFP)) {
----------------
kpn wrote:
> andrew.w.kaylor wrote:
> > This seemed odd if we're making the assumption that in general the BasicBlock IRBuilder is working with doesn't need to have a parent. So, I looked through the IRBuilder code a bit to see if we're assuming a parent exists anywhere else. We are. It happens all over the place. For example, IRBuilder::CreateAlloca() calls BB->getParent()->getParent() to get the Module.
> > 
> > While it might be reasonable to expect users to know that they can't set FP function attributes unless the IRBuilder is working with a BasicBlock that is already in a Function, it's more of a stretch to think that a user would know that about creating an AllocaInst. (The reason is that we need the DataLayout.) The same thing happens with any methods that create intrinsic calls.
> > 
> > I think that we should probably explicitly document that the IRBuilder must have a full chain from BasicBlock to Parent to Module.
> > 
> > That said, the changes in this patch seem reasonable.
> John did say above that "insertion into an unparented basic block" is a bug. And IRBuilder.cpp is littered with places that assume the block has a parent.
> 
> I still like the idea of the function definition getting the strictfp attribute automatically. And if we can't do that because of a clang bug then it seems like the right solution is to fix clang.
> 
> Is anyone working on the clang bug?
What are the conditions when the strictfp attribute should be added to a function?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70451/new/

https://reviews.llvm.org/D70451