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

[kpn]

Summary

The IRBuilder will currently automatically put the strictfp attribute on a function definition if strictfp mode is enabled and a function call is emitted. It turns out that this is incorrect since the IRBuilder is currently being used to add instructions to BasicBlocks that do _not_ have a Function. A segv is the result.

The new rule is that the front end must be the one to set the attribute on a function definition. Period. No attempt is made to be clever about trying to handle this in the IRBuilder automatically since it cannot be done with 100% reliably as far as I know.

This ticket is needed to unblock D62731.

[rjmccall]

This seems like a much more reasonable approach in general. Frontend authors who intend to use constrained FP need to be making broader changes to how they emit IR in order to satisfy the requirements of the intrinsics.

Have we been able to extract a test case which causes the insertion into an unparented basic block? If that's happening in Clang IRGen, I consider that a bug. I can't speak for LLVM passes, though; they do silly things for obscure reasons.

[kpn]

In D70451#1752027, @rjmccall wrote:

This seems like a much more reasonable approach in general. Frontend authors who intend to use constrained FP need to be making broader changes to how they emit IR in order to satisfy the requirements of the intrinsics.

Have we been able to extract a test case which causes the insertion into an unparented basic block? If that's happening in Clang IRGen, I consider that a bug. I can't speak for LLVM passes, though; they do silly things for obscure reasons.

The only one I know of is in this bug report: https://bugs.llvm.org/show_bug.cgi?id=44048

That's the bug that triggered the revert of D62731. I haven't spent enough time in clang to know how to turn this into a real test case for the unparented block issue.

[rjmccall]

That should be good enough.

[mibintc]

It works for me too. I need to send an update for my new test case fpconstrained.cpp
Thanks Kevin

[kpn]

In D70451#1752177, @mibintc wrote:

It works for me too. I need to send an update for my new test case fpconstrained.cpp

Note that since we're not going to be automatically putting this attribute on function definitions we'll need to update clang to do it hopefully soon. But that's another patch.

[andrew.w.kaylor]

llvm/include/llvm/IR/IRBuilder.h

262

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.

[kpn]

llvm/include/llvm/IR/IRBuilder.h

262

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?

[mibintc]

llvm/include/llvm/IR/IRBuilder.h

262

What are the conditions when the strictfp attribute should be added to a function?

[andrew.w.kaylor]

llvm/include/llvm/IR/IRBuilder.h

262

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.

[andrew.w.kaylor]

llvm/include/llvm/IR/IRBuilder.h

262

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.

[rjmccall]

llvm/include/llvm/IR/IRBuilder.h

262

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.

[mibintc]

llvm/include/llvm/IR/IRBuilder.h

262

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?

[andrew.w.kaylor]

llvm/include/llvm/IR/IRBuilder.h

262

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.

[erichkeane]

llvm/include/llvm/IR/IRBuilder.h

262

I looked into making sure that basic blocks are created with a function if we can, but it is unfortunately not that easy to do. We create basic blocks in a number of locations without a function, and there are a fairly sizable amount of asserts that ensure this is true.

I wonder if we would be better off adding a 'bit' to CodeGenFunction's state that this attribute must be set, then just set it in FinishFunction. It seems that would be WAY less of a change.

[mibintc]

llvm/include/llvm/IR/IRBuilder.h

262

@erichkeane I have a patch which does something like this, but the bit is somewhere else, I'll upload it soon, today.

[mibintc]

I uploaded another version of https://reviews.llvm.org/D62731 which sets the strictfp attribute during clang/CodeGen so it's not needed during IRBuilder, and doesn't cause the problem with the instruction block parent not being set. The new version makes a change to IRBuilder.h

[mibintc]

llvm/include/llvm/IR/IRBuilder.h

262

@rjmccall

This could fairly easily be changed ...

The problem is that we are holding an instruction and the IRBuilder wants to move from the instruction to the function containing the block. Since the parent field hasn't been set in the block, that won't work. I don't understand what you mean about moving the block to the end of the function, and/or I don't think it will solve this problem. Andy investigated and found out that several types of blocks are being created speculatively, possibly with null parent fields, and the blocks are discarded if not used. Even if we fix the TerminateLandingPad block, the other speculatively created blocks might also encounter this problem. Maybe the notion that it's possible to move from an instruction to the block parent is incorrect. Regardless, we can solve this particular IRBuilder problem a different way.

[rjmccall]

Well, I think it's best for IRBuilder to not be setting attributes on the function anyway, so I won't complain if you solve it another way.

[kpn]

@mibintc: This ticket is redundant now, correct? This was solved in the re-push of the command line option changes?

[mibintc]

In D70451#1775286, @kpn wrote:

@mibintc: This ticket is redundant now, correct? This was solved in the re-push of the command line option changes?

Yes!

[kpn]

Redundant now.

llvm/include/llvm/IR/IRBuilder.h

public:
   ConstrainedFPIntrinsic::RoundingMode getDefaultConstrainedRounding() {
     return DefaultConstrainedRounding;
   }
 
   void setConstrainedFPFunctionAttr() {
     assert(BB && "Must have a basic block to set any function attributes!");
 
     Function *F = BB->getParent();
+    assert(F && "Must have a function to set any function attributes!");
     if (!F->hasFnAttribute(Attribute::StrictFP)) {
       F->addFnAttr(Attribute::StrictFP);
     }
   }
 
   void setConstrainedFPCallAttr(CallInst *I) {
     if (!I->hasFnAttr(Attribute::StrictFP))
       I->addAttribute(AttributeList::FunctionIndex, Attribute::StrictFP);
-    setConstrainedFPFunctionAttr();
   }
 
   //===--------------------------------------------------------------------===//
   // RAII helpers.
   //===--------------------------------------------------------------------===//
 
   // RAII object that stores the current insertion point and restores it
   // when the object is destroyed. This includes the debug location.

llvm/unittests/IR/IRBuilderTest.cpp

TEST_F(IRBuilderTest, ConstrainedFP) {
   ASSERT_TRUE(isa<IntrinsicInst>(VDouble));
   II = cast<IntrinsicInst>(VDouble);
   EXPECT_EQ(II->getIntrinsicID(), Intrinsic::experimental_constrained_fpext);
 
   // Verify attributes on the call are created automatically.
   AttributeSet CallAttrs = II->getAttributes().getFnAttributes();
   EXPECT_EQ(CallAttrs.hasAttribute(Attribute::StrictFP), true);
 
-  // Verify attributes on the containing function are created automatically.
+  // Verify attributes on the containing function are created when requested.
+  Builder.setConstrainedFPFunctionAttr();
   AttributeList Attrs = BB->getParent()->getAttributes();
   AttributeSet FnAttrs = Attrs.getFnAttributes();
   EXPECT_EQ(FnAttrs.hasAttribute(Attribute::StrictFP), true);
 
   // Verify the codepaths for setting and overriding the default metadata.
   V = Builder.CreateFAdd(V, V);
   ASSERT_TRUE(isa<ConstrainedFPIntrinsic>(V));
   auto *CII = cast<ConstrainedFPIntrinsic>(V);