[PATCH] D68233: [FPEnv] [WIP] Verify strictfp attribute correctness

[Kevin P. Neal via Phabricator via llvm-commits]

kpn marked 3 inline comments as done.
kpn added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:359
     verifySiblingFuncletUnwinds();
+    verifyFunctionConstrainedFP(F);
     InstsInThisBlock.clear();
----------------
kbarton wrote:
> 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. 
Yes, this can be done.


================
Comment at: llvm/lib/IR/Verifier.cpp:2888
+  // Verify strictfp attributes match.
+  Function *ContainingF = Call.getFunction();
+  AttributeSet CFnAttrs = ContainingF->getAttributes().getFnAttributes();
----------------
arsenm wrote:
> Can check the attribute directly on Call? Also missing tests for this part?
So I can. Thanks for the nudge. And I checked that this is indeed tested.


================
Comment at: llvm/test/Verifier/fp-intrinsics.ll:94
+
+; Test for mismatched function and function call attributes
+; CHECK7: Functions and their contained calls must match in use of attribute strictfp!
----------------
arsenm wrote:
> Should have a real call for this, not the constrained intrinsic?
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.


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

https://reviews.llvm.org/D68233