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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
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*.
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 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 :-)
Can you give these an FPR prefix?