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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn marked an inline comment as done.
kpn 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)) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70451