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

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor 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)) {
----------------
mibintc wrote:
> 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?
> Is anyone working on the clang bug?

I've been poking at this to see why it was happening, but I'm definitely not the person to fix it. In the case that was failing it's happening as part of the exception handling code that generates a terminate landing pad.

In CodeGenFunction::getTerminateLandingPad() this is happening:

```
  // This will get inserted at the end of the function.
  TerminateLandingPad = createBasicBlock("terminate.lpad");
  Builder.SetInsertPoint(TerminateLandingPad);
```

That points the builder at an unparented block. A call to the system terminate function is inserted just below this. Eventually the builder's insertion point is restored to whatever it was. I haven't found the code yet that inserts the terminate block into the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70451