This is an archive of the discontinued LLVM Phabricator instance.

Extended FPOptions with new attributes
ClosedPublic

Authored by sepavloff on Aug 8 2019, 11:14 PM.

Details

Summary

This change added two new attributes, rounding mode and exception
behavior to the structure FPOptions. These attributes allow more
flexible treatment of specific floating point environment than it is
provided by fenv_access.

Diff Detail

Event Timeline

sepavloff created this revision.Aug 8 2019, 11:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 11:14 PM

In general, this seems reasonable, but is missing test code.

clang/include/clang/Basic/LangOptions.h
200

Can you give these an FPR prefix?

214

And these an FPE prefix?

kpn added a comment.Aug 9 2019, 8:13 AM

In general, this seems reasonable, but is missing test code.

The IRBuilder does have a strict FP mode setting now. When strict mode is enabled the (implemented) constrained intrinsics automatically replace the normal FP instructions. I wonder if that would be right for testing of this patch?

Since this setting changes IR output, you should write a test that emits IR for source code and tests that you get the right IR output.

sepavloff updated this revision to Diff 214754.Aug 12 2019, 8:35 PM

Updated patch

sepavloff marked 2 inline comments as done.Aug 12 2019, 8:46 PM

Since this setting changes IR output, you should write a test that emits IR for source code and tests that you get the right IR output.

This patch is a part of patch chain, it extends FPOptions with new options. In D65997 pragma clang fp is extended to manipulate these options. Finally D66092 modifies code generator so that it emit IR depending on the new options in FPOptions, it makes possible to write IR tests.

In D65994#1623088, @kpn wrote:

In general, this seems reasonable, but is missing test code.

The IRBuilder does have a strict FP mode setting now. When strict mode is enabled the (implemented) constrained intrinsics automatically replace the normal FP instructions. I wonder if that would be right for testing of this patch?

It is just what D66092 does.

Oh, sorry, I confused several patches, then.

I don't actually think you need to break down the patches this finely; it would be fine to take all three steps to implement the feature in one pass. It's only important to break down the patches into more incremental components if there's significant refactoring required to prepare for the patch or if each step is substantially complex on its own. And it's nice for feature work to always be testable *somehow*.

sepavloff updated this revision to Diff 239893.Jan 23 2020, 7:07 AM

Rebased the patch

aaron.ballman accepted this revision.Jan 25 2020, 11:14 AM

LGTM, but I'll echo what @rjmccall said about this level of granularity being a bit too fine for review for future patches (we expect changes to be testable, which this is, but only if you also review other patches and notice the tests and how they relate to this patch, which can be easy for reviewers to miss).

This revision is now accepted and ready to land.Jan 25 2020, 11:14 AM
This revision was automatically updated to reflect the committed changes.

This patch increased the used size of BinaryOperator by 5 bits.
Those bits were all padding, but now BinaryOperatorBitfields is completely full at 32 bits and we can't add any new bits to Stmt without increasing BinaryOperator by 8 bytes. (See D75443 and D54526 for the optimization this would revert).

To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7 bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not really familiar with this domain - if many of the 90 states are not possible it'd probably be useful to have some more bits back :-)