Page MenuHomePhabricator

Extended FPOptions with new attributes
Needs ReviewPublic

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.

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
188

Can you give these an FPR prefix?

202

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*.