This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kpn on Mar 24 2023, 1:52 PM.

Details

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.

Diff Detail

Event Timeline

kpn created this revision.Mar 24 2023, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 1:52 PM
kpn requested review of this revision.Mar 24 2023, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 1:52 PM
arsenm added inline comments.Mar 24 2023, 4:33 PM
llvm/lib/IR/Verifier.cpp
3340–3343

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

arsenm added inline comments.Mar 24 2023, 4:37 PM
llvm/test/Verifier/fp-intrinsics.ll
170

Test indirect call? and alias?

kpn updated this revision to Diff 511147.Apr 5 2023, 10:07 AM

Update for review comments.

kpn updated this revision to Diff 511786.Apr 7 2023, 1:37 PM

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

arsenm added inline comments.Apr 7 2023, 3:14 PM
llvm/lib/IR/Verifier.cpp
6101

Drop "== true"

llvm/test/Verifier/fp-intrinsics.ll
95–99

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 requested changes to this revision.Jun 22 2023, 10:23 AM
arsenm added inline comments.
llvm/test/Verifier/fp-intrinsics.ll
95–99

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

224

Needs to update to opaque pointers

This revision now requires changes to proceed.Jun 22 2023, 10:23 AM
kpn updated this revision to Diff 542509.Jul 20 2023, 7:12 AM

Update for review comments.

arsenm accepted this revision.Jul 20 2023, 8:25 AM
This revision is now accepted and ready to land.Jul 20 2023, 8:25 AM
kpn updated this revision to Diff 553187.Aug 24 2023, 10:40 AM

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 added inline comments.Aug 24 2023, 1:13 PM
llvm/lib/IR/Verifier.cpp
3340–3343

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

3346

lowercase strictfp to match the name

kpn updated this revision to Diff 553557.Aug 25 2023, 11:40 AM

Update for review comments.

kpn added a comment.Sep 13 2023, 5:29 AM

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

arsenm accepted this revision.Sep 13 2023, 7:00 AM