This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] [WIP] Verify strictfp attribute correctness
Needs RevisionPublic

Authored by kpn on Sep 30 2019, 10:17 AM.

Details

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.

Diff Detail

Event Timeline

kpn created this revision.Sep 30 2019, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 10:17 AM
kpn updated this revision to Diff 225937.Oct 21 2019, 12:14 PM

Add strict attribute checks to all function calls.

Needs tests

simoll added a subscriber: simoll.Nov 4 2019, 9:38 AM
pengfei added a subscriber: pengfei.Nov 4 2019, 9:26 PM
kpn updated this revision to Diff 233146.Dec 10 2019, 10:47 AM

Rebase. Add missing tests.

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

arsenm added inline comments.Apr 2 2020, 4:49 PM
llvm/lib/IR/Verifier.cpp
2884

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

llvm/test/Verifier/fp-intrinsics.ll
94 ↗(On Diff #233146)

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

kbarton added inline comments.May 1 2020, 11:04 AM
llvm/lib/IR/Verifier.cpp
358

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 marked 3 inline comments as done.May 28 2020, 7:50 AM
kpn added inline comments.
llvm/lib/IR/Verifier.cpp
358

Yes, this can be done.

2884

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

llvm/test/Verifier/fp-intrinsics.ll
94 ↗(On Diff #233146)

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 updated this revision to Diff 266881.May 28 2020, 7:51 AM

Address review comments.

kpn updated this revision to Diff 345190.May 13 2021, 9:49 AM

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 requested changes to this revision.Jan 12 2023, 4:44 AM
arsenm added inline comments.
llvm/lib/IR/Verifier.cpp
2886

I think these Asserts were renamed to Check

2887

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 ↗(On Diff #345190)

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

This revision now requires changes to proceed.Jan 12 2023, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 4:44 AM
Herald added a subscriber: StephenFan. · View Herald Transcript