[FPEnv] [WIP] Verify strictfp attribute correctness, first part, 2023 edition

[kpn]

Summary

This is a rewrite of D68233. Enough of the patch has been rewritten in the two years that I think it makes sense to just open a new review. I've copied over the reviewers and subscribers, and arsenm had a blocking request on D68233 that I've brought over here.

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 or called functions agree with their contained function about the strictfp attribute. It also enforces the attribute if a constrained fp intrinsic is used in a function.

Notably, the attribute is now allowed to be omitted from a call site if it is present on the declaration for the function being called. This is a change from the current Language Reference and the Langref should be changed in a subsequent patch if this change is accepted here.

This patch 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.

[arsenm]

llvm/lib/IR/Verifier.cpp

3322–3325

Should go through the CallBase queries? getCalledFunction doesn't work for aliases and indirect calls

[arsenm]

llvm/test/Verifier/fp-intrinsics.ll

129

Test indirect call? and alias?

[kpn]

Update for review comments.

[kpn]

Verify that CallBase::isStrictFP() continues to do what we think it is doing.

[arsenm]

llvm/lib/IR/Verifier.cpp

6030

Drop "== true"

llvm/test/Verifier/fp-intrinsics.ll

54–58

Something is wrong here, there should be no error. The parent function has strictfp, and strictfp should always be implied by the intrinsic definition

[arsenm]

llvm/test/Verifier/fp-intrinsics.ll

54–58

I think I see the issue, the constrained intrinsics are not marked strictfp.

This is related to the issue I posted about here. We should have some kind of fpenv access attribute that indicates whether an intrinsic may read or write the mode such that the verifier can more systematically enforce this. It would be nicer than having to separately update ConstrainedOps.def

183

Needs to update to opaque pointers

[kpn]

Update for review comments.

[kpn]

Weaken the test. Now it only checks for the strictfp attribute on function calls if is found on the containing function definition. A call with the strictfp attribute in a function that lacks the strictfp attribute is allowed. Since we're also disallowing use of the constrained intrinsics without the strictfp attribute on the function definition the weakened test should be just as good as the previous iteration of this patch.

With this change to this patch I can abandon D157766. A failing Inliner test is also brought back by this change.

[arsenm]

llvm/lib/IR/Verifier.cpp

3322–3325

Still should just use the CallBase queries to avoid doing the attribute set logic yourself

3328

lowercase strictfp to match the name

[kpn]

Update for review comments.

[kpn]

Ping. Is this revised version still accepted? @arsenm?

llvm/lib/IR/Verifier.cpp

Check(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, Call.isInlineAsm());
 
+  // Verify strictfp attributes match.
+  Function *ContainingF = Call.getFunction();
+  Function *CalledF = Call.getCalledFunction();
+  bool ContainingFHasStrictFP = ContainingF->hasFnAttribute(Attribute::StrictFP);
+  AttributeSet CallAttrs = Attrs.getFnAttrs();
+  bool CallHasStrictFP = CallAttrs.hasAttribute(Attribute::StrictFP);
+  bool CalledFHasStrictFP = false;
+  if (CalledF) {
+    CalledFHasStrictFP = CalledF->hasFnAttribute(Attribute::StrictFP);
+  }
+  Check(ContainingFHasStrictFP == (CallHasStrictFP || CalledFHasStrictFP),
+        "Functions and their contained calls and invokes 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()))
       Check(AI->isUsedWithInAlloca(),
             "inalloca argument for call has mismatched alloca", AI, Call);
if (VPI.getIntrinsicID() == Intrinsic::vp_icmp) {
     Check(CmpInst::isIntPredicate(Pred),
           "invalid predicate for VP integer comparison intrinsic", &VPI);
   }
 }
 
 void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
   unsigned NumOperands;
   bool HasRoundingMD;
+  Check(FPI.getCaller()->hasFnAttribute(Attribute::StrictFP) == true,
+        "Constrained FP intrinsics require functions have strictfp attribute.",
+        FPI.getCaller());
   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

 entry:
   %fadd = call double @llvm.experimental.constrained.sqrt.f64(
                                         double %a,
                                         metadata !"round.dynamic",
                                         metadata !"fpexcept.restrict") #0
   ret double %fadd
 }
 
+; Test for mismatched function and function call attributes
+; CHECK-NEXT: Functions and their contained calls and invokes must match in use of attribute strictfp!
+; CHECK-NEXT: ptr @f6
+; CHECK-NEXT:  %fadd = call double @llvm.experimental.constrained.sqrt.f64(double %a, metadata !"round.dynamic", metadata !"fpexcept.strict")
+define double @f6(double %a) #0 {
+entry:
+  %fadd = call double @llvm.experimental.constrained.sqrt.f64(
+                                          double %a,
+                                          metadata !"round.dynamic",
+                                          metadata !"fpexcept.strict")
+  ret double %fadd
+}
+
+; Test for mismatched function and function call attributes
+; CHECK-NEXT: Functions and their contained calls and invokes must match in use of attribute strictfp!
+; CHECK-NEXT: ptr @f7
+; CHECK-NEXT:  %fadd = call double @llvm.experimental.constrained.sqrt.f64(double %a, metadata !"round.dynamic", metadata !"fpexcept.strict") #1
+define double @f7(double %a) {
+entry:
+  %fadd = call double @llvm.experimental.constrained.sqrt.f64(
+                                          double %a,
+                                          metadata !"round.dynamic",
+                                          metadata !"fpexcept.strict") #0
+  ret double %fadd
+}
+
+; Test that strictfp attribute on function declaration counts.
+; No diagnostic should be printed.
+declare double @arbitrarystrictfp.f64(double) #0
+define double @f8(double %a) #0 {
+entry:
+  %fadd = call double @arbitrarystrictfp.f64(double %a)
+  ret double %fadd
+}
+
+; Test that strictfp attribute on function declaration counts.
+; CHECK-NEXT: Functions and their contained calls and invokes must match in use of attribute strictfp!
+; CHECK-NEXT: ptr @f9
+; CHECK-NEXT: %fadd = call double @arbitrarystrictfp.f64(double %a)
+define double @f9(double %a) {
+entry:
+  %fadd = call double @arbitrarystrictfp.f64(double %a)
+  ret double %fadd
+}
+
+; Test for mismatched function and function call attributes
+; CHECK-NEXT: Functions and their contained calls and invokes must match in use of attribute strictfp!
+; CHECK-NEXT: ptr @f10
+; CHECK-NEXT: %sqrt = call double @llvm.sqrt.f64(double %a)
+declare double @llvm.sqrt.f64(double)
+define double @f10(double %a) #0 {
+entry:
+  %sqrt = call double @llvm.sqrt.f64(double %a)
+  ret double %sqrt
+}
+
+; Test for mismatched function and (invoke) function call attributes
+; CHECK-NEXT: Functions and their contained calls and invokes must match in use of attribute strictfp!
+; CHECK-NEXT: ptr @f100
+; CHECK-NEXT:   %0 = invoke double @doublefoo(double %a)
+; CHECK-NEXT:           to label %conta unwind label %contb
+declare double @doublefoo(double)
+declare i32 @__gxx_personality_v0(...)
+define void @f100(double %a) #0 personality ptr @__gxx_personality_v0 {
+entry:
+  invoke double @doublefoo(double %a)
+    to label %conta unwind label %contb
+
+conta:
+  ret void
+
+contb:
+  %1 = landingpad { i8*, i32 }
+          filter [0 x i8*] zeroinitializer
+  ret void
+}
+
 attributes #0 = { strictfp }