This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] Strict FP tests should use the requisite function attributes
ClosedPublic

Authored by kpn on Sep 23 2019, 10:21 AM.

Details

Summary

A set of function attributes is required in any function that uses constrained floating point intrinsics. None of our tests use these attributes. I've got IR verifier changes in the works that enforce use of them, but first the existing tests need to be made compliant.

Diff Detail

Repository
rL LLVM

Event Timeline

kpn created this revision.Sep 23 2019, 10:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 10:21 AM

This looks fine to me, assuming we agree that this is in fact the set of attributes we need. So I think https://reviews.llvm.org/D67839 should go in first.

Would it be better to do an attributes #0 = { ...} style change? E.g.:

define double @f5(double %a) #0 {
...
}

attributes #0 = {strictfp noimplicitfloat}
kpn added a comment.Sep 24 2019, 12:19 PM

Would it be better to do an attributes #0 = { ...} style change? E.g.:

define double @f5(double %a) #0 {
...
}

attributes #0 = {strictfp noimplicitfloat}

I like that syntax since it is much more compact. I imagine I'll need to update the tests after D67839 is concluded. I'll give this syntax a spin then.

kpn planned changes to this revision.Sep 26 2019, 11:08 AM

This needs to be updated to mark function calls now that D67839 is done.

kpn updated this revision to Diff 223039.Oct 3 2019, 9:33 AM

Based on the conclusion from D67839, I've updated the tests to have function attribute strictfp always be used when constrained intrinsics are used, and all function calls are also marked strictfp when the function itself is marked strictfp.

There are still three tests that aren't correct. Those will be subsequent patches.

The verifier changes are in D68233. It can't go in until all tests are passing with it.

SystemZ changes still LGTM.

andrew.w.kaylor accepted this revision.Oct 3 2019, 1:21 PM

I assume you've tested this with your verifier changes. This is a bit too much to verify every call and function manually. I looked at the ones that weren't boilerplate updates, and I sampled the boilerplate changes. Based on that review strategy, it looks good to me.

This revision is now accepted and ready to land.Oct 3 2019, 1:21 PM
This revision was automatically updated to reflect the committed changes.