This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] Document that constrained FP intrinsics cannot be mixed with non-constrained
ClosedPublic

Authored by kpn on Sep 9 2019, 9:52 AM.

Details

Summary

It does not appear that we've documented that constrained FP intrinsics cannot be mixed in a single function with non-constrained instructions. This patch fixes that.

Diff Detail

Event Timeline

kpn created this revision.Sep 9 2019, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 9:52 AM

My understanding was that this is a temporary restriction; in the end, the goal is to allow mixing constrained an non-constrained operations, once we have the necessary barriers. Did I misunderstand this?

Now, I guess it still makes sense to document the restriction even if it is temporary, I'm just wondering whether this should be made explicit in the docs.

kpn added a comment.Sep 12 2019, 8:10 AM

My understanding was that this is a temporary restriction; in the end, the goal is to allow mixing constrained an non-constrained operations, once we have the necessary barriers. Did I misunderstand this?

Now, I guess it still makes sense to document the restriction even if it is temporary, I'm just wondering whether this should be made explicit in the docs.

I can see how knowing that a change to add FP barriers may be coming may affect designs. I believe Serge Pavlov mentioned that he was going to have to "rework" (his word) his patches since he didn't know about the restriction at all. So more information is probably better. I'll update after lunch.

kpn updated this revision to Diff 219937.Sep 12 2019, 9:24 AM

Address review comment. Mention the possibility of the restrictions mentioned here changing in the future, but don't make any promises.

My understanding was that this is a temporary restriction; in the end, the goal is to allow mixing constrained an non-constrained operations, once we have the necessary barriers. Did I misunderstand this?

I think the consensus was that there is no good way to prevent hoisting of non-constrained FP arith ops, since they are side-effect free.

In D67360#1668007, @kpn wrote:

Address review comment. Mention the possibility of the restrictions mentioned here changing in the future, but don't make any promises.

I don't see any benefit in mentioning that. I agree with Cameron that there is currently no good way to prevent hoisting, and I don't think we want for there to be a way to do that because an explicit goal of the constrained implementation was to ensure that it didn't interfere with optimization of code that didn't require any restrictions, which we believe will always be the majority case by a considerable margin. I expect that mixing of constrained and non-constrained regions will be rather rare. The use of intrinsics is the mechanism we've chosen to restrict code motion.

My understanding of our long term goal is that we will teach optimizations to handle the intrinsics, so that when optimizations are possible (within the constraints described by the intrinsic) they will be performed. Once we reach that state, there will be no serious downside to using an intrinsic with the most permissive settings versus a raw FP operation.

docs/LangRef.rst
15062

As long as you're cleaning this up, I would suggest ditching the ordinal adjectives (which I never should have used in the original documentation as the problem you're fixing here should have been foreseeable). How about saying "the data operands and the return value" here and referring to "rounding mode argument" and "exception behavior" argument below. You can't really say "next" and "last" there since they aren't always both used.

Some additional rewriting will be necessary to avoid saying "the rounding mode argument is a metadata argument specifying the rounding mode...." Here's a suggestion:

The rounding mode argument is a metadata string specifying what assumptions, if any, the optimizer can make when transforming constant values. Some constrained FP intrinsics omit this argument. If required by the intrinsic, this argument must be one of the following strings:

<...>

The exception behavior argument is a metadata string describing the floating point exception semantics that required for the intrinsic. This argument must be one of the following strings:

kpn updated this revision to Diff 220095.Sep 13 2019, 6:45 AM

Incorporate changes and text requested by Andrew Kaylor. Remove mention of the lack of barriers and include text on how to accomplish mixing without barriers.

kpn marked an inline comment as done.Sep 13 2019, 6:46 AM
andrew.w.kaylor accepted this revision.Sep 13 2019, 12:24 PM

lgtm

Thanks for updating this bit of documentation!

This revision is now accepted and ready to land.Sep 13 2019, 12:24 PM
This revision was automatically updated to reflect the committed changes.