[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:
> rjmccall wrote:
> > andrew.w.kaylor wrote:
> > > andrew.w.kaylor wrote:
> > > > 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.
> > > I have a bit more information. The TerminateLandingPad block is created speculatively. Apparently there are circumstances under which it might be created but not used, in which case it will be deleted during CodeGenFunction::FinishFunction(). A number of special blocks are handled this way: EHResumeBlock, TerminateLandingPad, TerminateHandler, UnreachableBlock, and any TerminateFunclets. For each of these, we call EmitIfUsed() which will either insert the block into the current function or delete it.
> > > 
> > > So, that's why we're creating the block and populating it without it having a parent. I guess we're just never creating allocas or intrinsic calls in these special blocks.
> > > 
> > > This leaves me certain of what I said before about me not being the right person to fix this. I feel like I understand the problem now, but I'd just be guessing at the right solution. My guess would be that we could insert the blocks when they are created and change EmitIfUsed() to DeleteIfUnused(), but I don't know clang well enough to know how that might affect other parts of the code.
> > > 
> > > > What are the conditions when the strictfp attribute should be added to a function?
> > > 
> > > In theory, if any part of the function was translated with the constrained FP mode enabled, the function should have the strict FP attribute. More practically, if the function contains any calls to constrained intrinsics or call sites with strictfp set, it should have the strict FP attribute. These are mostly equivalent, but there is some ambiguity in the case where the constrained mode is enabled but there are no function calls or FP operations.
> > > I haven't found the code yet that inserts the terminate block into the function.
> > 
> > `CodeGenFunction::FinishFunction`.  This could fairly easily be changed to move the block to the end of the function if it's used, though.
> > 
> > 
> Thanks @andrew.w.kaylor so, if a function contains use of constrained intrinsics, then that function should have the strict FP attribute.  What call sites get marked with strictfp attribute?  If it's a call to a function with strict fp?
> What call sites get marked with strictfp attribute? If it's a call to a function with strict fp?

No, all call sites within a strictfp function definition should be marked with the strictfp attribute. The purpose is to prevent calls to math library functions from being optimized contrary to the dynamic floating point environment. But we don't want the front end to be responsible for identifying which library calls we might optimize, so it should just mark all calls.

We probably should also attach the actual constraints, but we don't have a mechanism for that. My recent operand bundle proposal is sort of what I have in mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70451