[FPEnv] [WIP] Verify strictfp attribute correctness

[kpn]

Summary

The strictfp attribute is now defined to require that function definitions be marked strictfp if they contain a strictfp call or a strictfp constrained intrinsic. This patch verifies that all function calls agree with their contained function about the strictfp attribute. It also enforces the attribute if a constrained fp intrinsic is used in a function.

It does _not_ require that all FP instructions in a function be strict if any of this is. That's a later patch.

Note that this patch _cannot_ yet go in the tree because quite a few tests break with this check in place. I'm working on fixing or getting those fixed, and having this patch visible is part of that.

[kpn]

Add strict attribute checks to all function calls.

[arsenm]

Needs tests

[kpn]

Rebase. Add missing tests.

Currently blocked on D70096. Other known/new test failures should be easy to fix.

[arsenm]

llvm/lib/IR/Verifier.cpp

2973

Can check the attribute directly on Call? Also missing tests for this part?

llvm/test/Verifier/fp-intrinsics.ll

95

Should have a real call for this, not the constrained intrinsic?

[kbarton]

llvm/lib/IR/Verifier.cpp

359

I don't know how the traversal is done, but is this something that could be done inside visitFunction instead?
That would require all of the instructions have been traversed at that point, and all the intrinsics have been visited, and I'm not sure whether that happens before or ofter visitFunction is called.

[kpn]

llvm/lib/IR/Verifier.cpp

359

Yes, this can be done.

2973

So I can. Thanks for the nudge. And I checked that this is indeed tested.

llvm/test/Verifier/fp-intrinsics.ll

95

Well, D70096 will change the "strictfp" to a "nobuiltin" in this case here. I think I'm going to change D70096 so that it leaves strictfp alone on a constrained intrinsic. That way it will fail here with this test. But a real function call will get converted to "nobuiltin" and thus we won't be testing here what we need to be testing.

[kpn]

Address review comments.

[kpn]

Rebase.

We're still not ready to push this, but it can be used to find issues in clang. Once all tests pass with this change we can push it. We're not there yet.

[arsenm]

llvm/lib/IR/Verifier.cpp

2975

I think these Asserts were renamed to Check

2976

Shouldn't require the callsite attributes to match if the underlying declaration is strictfp, your test doesn't cover that case

llvm/test/Verifier/fp-intrinsics.ll

2–9

I don't understand the sed usage of this test. Verifier checks can all coexist in the same module. This test only needs a single run line with no sed

llvm/lib/IR/Verifier.cpp

class Verifier : public InstVisitor<Verifier>, VerifierSupport {
 
   /// The result type for a landingpad.
   Type *LandingPadResultTy;
 
   /// Whether we've seen a call to @llvm.localescape in this function
   /// already.
   bool SawFrameEscape;
 
+  /// Whether or not we've seen constrained floating-point intrinsics
+  /// in a function.
+  bool HasConstrainedFP;
+
   /// Whether the current function has a DISubprogram attached to it.
   bool HasDebugInfo = false;
 
   /// Whether source was present on the first DIFile encountered in each CU.
   DenseMap<const DICompileUnit *, bool> HasSourceDebugInfo;
 
   /// Stores the count of how many objects were passed to llvm.localescape for a
   /// given function and the largest index passed to llvm.localrecover.
for (const BasicBlock &BB : F) {
       }
       return false;
     }
 
     Broken = false;
     // FIXME: We strip const here because the inst visitor strips const.
     visit(const_cast<Function &>(F));
     verifySiblingFuncletUnwinds();
     InstsInThisBlock.clear();
     DebugFnArgs.clear();
     LandingPadResultTy = nullptr;
     SawFrameEscape = false;
     SiblingFuncletInfo.clear();
 
     return !Broken;
   }
 
for (const auto &Pair : SiblingFuncletInfo) {
     // nodes' successors.
     Active.clear();
   }
 }
 
 // visitFunction - Verify that a function is ok.
 //
 void Verifier::visitFunction(const Function &F) {
+  HasConstrainedFP = false;
+
   visitGlobalValue(F);
 
   // Check function arguments.
   FunctionType *FT = F.getFunctionType();
   unsigned NumArgs = F.arg_size();
 
   Assert(&Context == &F.getContext(),
          "Function context does not match Module context!", &F);
void Verifier::visitFunction(const Function &F) {
   // Only do this if the module is materialized, otherwise we don't have all the
   // uses.
   if (F.getIntrinsicID() && F.getParent()->isMaterialized()) {
     const User *U;
     if (F.hasAddressTaken(&U))
       Assert(false, "Invalid user of intrinsic instruction!", U);
   }
 
+  // Verify that the function has the correct attributes if a constrained
+  // floating-point value was seen.
+  if (HasConstrainedFP)
+    Assert (F.hasFnAttribute(Attribute::StrictFP),
+            "Constrained floating point requires function attribute strictfp!",
+            &F);
+
   auto *N = F.getSubprogram();
   HasDebugInfo = (N != nullptr);
   if (!HasDebugInfo)
     return;
 
   // Check that all !dbg attachments lead to back to N.
   //
   // FIXME: Check this incrementally while visiting !dbg attachments.
Assert(Call.getCalledFunction()->getIntrinsicID() ==
                Intrinsic::call_preallocated_arg,
            "preallocated as a call site attribute can only be on "
            "llvm.call.preallocated.arg");
   }
 
   // Verify call attributes.
   verifyFunctionAttrs(FTy, Attrs, &Call, IsIntrinsic);
 
+  // Verify strictfp attributes match.
+  Function *ContainingF = Call.getFunction();
+  AttributeSet CallAttrs = Attrs.getFnAttributes();
+  Assert (ContainingF->hasFnAttribute(Attribute::StrictFP) ==
+          CallAttrs.hasAttribute(Attribute::StrictFP),
+            "Functions and their contained calls must match "
+            "in use of attribute strictfp!",
+            ContainingF, &Call);
+
   // Conservatively check the inalloca argument.
   // We have a bug if we can find that there is an underlying alloca without
   // inalloca.
   if (Call.hasInAllocaArgument()) {
     Value *InAllocaArg = Call.getArgOperand(FTy->getNumParams() - 1);
     if (auto AI = dyn_cast<AllocaInst>(InAllocaArg->stripInBoundsOffsets()))
       Assert(AI->isUsedWithInAlloca(),
              "inalloca argument for call has mismatched alloca", AI, Call);
static DISubprogram *getSubprogram(Metadata *LocalScope) {
   // Just return null; broken scope chains are checked elsewhere.
   assert(!isa<DILocalScope>(LocalScope) && "Unknown type of local scope");
   return nullptr;
 }
 
 void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
   unsigned NumOperands;
   bool HasRoundingMD;
+  HasConstrainedFP = true;
+  AttributeSet CallAttrs = FPI.getAttributes().getFnAttributes();
+  Assert (CallAttrs.hasAttribute(Attribute::StrictFP),
+          "Constrained FP intrinsics require strictfp attribute.", &FPI);
   switch (FPI.getIntrinsicID()) {
 #define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC)                         \
   case Intrinsic::INTRINSIC:                                                   \
     NumOperands = NARG;                                                        \
     HasRoundingMD = ROUND_MODE;                                                \
     break;
 #include "llvm/IR/ConstrainedOps.def"
   default:

llvm/test/Verifier/fp-intrinsics.ll

 ; RUN: opt -verify -S < %s 2>&1 | FileCheck --check-prefix=CHECK1 %s
 ; RUN: sed -e s/.T2:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK2 %s
 ; RUN: sed -e s/.T3:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK3 %s
 ; RUN: sed -e s/.T4:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK4 %s
 ; RUN: sed -e s/.T5:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK5 %s
+; RUN: sed -e s/.T6:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK6 %s
+; RUN: sed -e s/.T7:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK7 %s
+; RUN: sed -e s/.T8:// %s | not opt -verify -disable-output 2>&1 | FileCheck --check-prefix=CHECK8 %s
 
 ; Common declarations used for all runs.
 declare double @llvm.experimental.constrained.fadd.f64(double, double, metadata, metadata)
+declare float @llvm.experimental.constrained.fadd.f32(float, float, metadata, metadata)
 declare double @llvm.experimental.constrained.sqrt.f64(double, metadata, metadata)
 
 ; Test that the verifier accepts legal code, and that the correct attributes are
 ; attached to the FP intrinsic.
 ; CHECK1: declare double @llvm.experimental.constrained.fadd.f64(double, double, metadata, metadata) #[[ATTR:[0-9]+]]
 ; CHECK1: declare double @llvm.experimental.constrained.sqrt.f64(double, metadata, metadata) #[[ATTR]]
 ; CHECK1: attributes #[[ATTR]] = { inaccessiblememonly nounwind willreturn }
 ; Note: FP exceptions aren't usually caught through normal unwind mechanisms,

 ;T5: entry:
 ;T5:   %fadd = call double @llvm.experimental.constrained.sqrt.f64(
 ;T5:                                         double %a,
 ;T5:                                         metadata !"round.dynamic",
 ;T5:                                         metadata !"fpexcept.restrict") #0
 ;T5:   ret double %fadd
 ;T5: }
 
+; Test for mismatched function and function call attributes
+; CHECK6: Functions and their contained calls must match in use of attribute strictfp!
+;T6: define double @f6(double %a) #0 {
+;T6: entry:
+;T6:   %fadd = call double @llvm.experimental.constrained.sqrt.f64(
+;T6:                                           double %a,
+;T6:                                           metadata !"round.dynamic",
+;T6:                                           metadata !"fpexcept.strict")
+;T6:   ret double %fadd
+;T6: }
+
+; Test for mismatched function and function call attributes
+; CHECK7: Functions and their contained calls must match in use of attribute strictfp!
+;T7: declare double @llvm.sqrt.f64(double)
+;T7: define double @f7(double %a) #0 {
+;T7: entry:
+;T7:   %sqrt = call double @llvm.sqrt.f64(double %a)
+;T7:   ret double %sqrt
+;T7: }
+
+; Test for mismatched function and function call attributes
+; CHECK8: Functions and their contained calls must match in use of attribute strictfp!
+;T8: define double @f8(double %a) {
+;T8: entry:
+;T8:   %fadd = call double @llvm.experimental.constrained.sqrt.f64(
+;T8:                                           double %a,
+;T8:                                           metadata !"round.dynamic",
+;T8:                                           metadata !"fpexcept.strict") #0
+;T8:   ret double %fadd
+;T8: }
+
 attributes #0 = { strictfp }