Page MenuHomePhabricator

Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress
AcceptedPublic

Authored by mibintc on Sep 11 2020, 10:09 AM.

Details

Summary

I rebased https://reviews.llvm.org/D69272 to work on the blocking comment from @rsmith
@sepavloff Is it OK if I continue work on this item? Not sure about the protocol when continuing someone else's patch.

@rsmith asked "I don't see changes to the constant evaluator". The semantic rules for enabling this pragma require that strict or precise semantics be in effect., see SemaAttr.cpp And the floating point constant evaluation doesn't occur when strict or precise

One of the test cases in this patch shows 1+2 isn't folded in strict mode.

However, I don't see where is the decision about floating point constant folding, can someone tell me where that occurs? . I thought the constant folding was in AST/ExprConstant.cpp and indeed constant folding does occur there. But sometimes, if the floating point semantics are set to 'strict', even tho' folding has occurred successfully in ExprConstant.cpp, when i look at emit-llvm, there is arithmetic emitted for the floating point expression; For example if you use the command line option -ffp-exception-behavior=strict and you compile this function, it will emit the add instruction; but without the option you will see the folded expression 3.0. Either way (i.e. regardless of option) if you put a breakpoint inside ExprConstant.cpp the calculation of the floating sum does occur. The function is float myAdd(void) { return 1.0 + 2.0; }

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Sep 14 2020, 5:15 PM
clang/lib/AST/ExprConstant.cpp
824–827

This is a property of the operation being constant-evaluated, not of the EvalInfo. The way you compute it below will only pick up the right value if the top-level expression in the evaluation happens to be a constrained floating-point evaluation.

13481–13482

I think we should be able to evaluate (for example) constexpr float f = 1.0f; even in a strict FP context. I think only floating point operations that depend on the rounding mode should be disabled, not all floating-point evaluation. Perhaps we should propagate the FPOptions into handleFloatFloatBinOp and perform the check there, along with any other places that care (I think we probably have some builtins that we can constant-evaluate that care about rounding modes.)

You also need to produce a diagnostic when you treat an expression as non-constant -- please read the comment at the top of the file for details.

This revision now requires changes to proceed.Sep 14 2020, 5:15 PM
mibintc updated this revision to Diff 292343.Sep 16 2020, 2:27 PM

I pulled out the isStrictFP boolean from EvalInfo and used the FPOptions when visiting floating point BinaryOperator, CastExpr and builtin CallExpr. I created the diagnostic note explaining why constant evaluation failed. Not certain about the language rules C vs C++.

mibintc added inline comments.Sep 16 2020, 2:29 PM
clang/lib/AST/ExprConstant.cpp
13481–13482

I think we should be able to evaluate (for example) constexpr float f = 1.0f; even in a strict FP context. I think only floating point operations that depend on the rounding mode should be disabled, not all floating-point evaluation. Perhaps we should propagate the FPOptions into handleFloatFloatBinOp and perform the check there, along with any other places that care (I think we probably have some builtins that we can constant-evaluate that care about rounding modes.)

You also need to produce a diagnostic when you treat an expression as non-constant -- please read the comment at the top of the file for details.

It's not just rounding mode, right? If fp exceptions are unmasked then we should inhibit the folding in particular cases?

rsmith added inline comments.Sep 16 2020, 5:51 PM
clang/lib/AST/ExprConstant.cpp
2843

E here is only intended for use in determining diagnostic locations, and isn't intended to necessarily be the expression being evaluated. Instead of assuming that this is the right expression to inspect, please either query the FP features in the caller and pass them into here or change this function to take a const BinaryOperator*.

2845

This should be an FFDiag not a CCEDiag because we want to suppress all constant folding, not only treating the value as a core constant expression. Also we should check this before checking for a NaN result, because if both happen at once, this is the diagnostic we want to produce.

13572–13575

Hm, does fabs depend on the floating-point environment? Is the concern the interaction with signaling NaNs?

13658–13662

Is it feasible to only reject cases that would actually be inexact, rather than disallowing all conversions to floating-point types? It would seem preferable to allow at least widening FP conversions and complex -> real, since they never depend on the FP environment (right?).

clang/test/CodeGen/pragma-fenv_access.c
2

Is this RUN line intentionally disabled?

mibintc updated this revision to Diff 294074.Thu, Sep 24, 8:42 AM

Responded to @rsmith's review

Note, @sepavloff is working on overlapping concerns here, D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

mibintc marked 2 inline comments as done.Thu, Sep 24, 8:51 AM
mibintc added inline comments.
clang/lib/AST/ExprConstant.cpp
2843

OK I changed it to use BinaryOperator

2845

OK I made this change

13572–13575

You're right, "fabs(x) raises no floating-point exceptions, even if x is a signaling NaN. The returned value is independent of the current rounding direction mode.". Thanks a lot for the careful reading, I really appreciate it

13658–13662

I changed it like you suggested, do the binary APFloat calculation and check the operation status to see if the result is inexact. I also added logic to check "is widening". Is that OK (builtin kind <) or maybe I should rather check the precision bitwidth?

clang/test/CodeGen/pragma-fenv_access.c
2

Oops thank you. I was working with an old revision of the patch and the test cause no longer worked because rounding setting was different. i just got rid of the run line that includes frounding-math

mibintc marked 2 inline comments as done.Thu, Sep 24, 10:54 AM
sepavloff added inline comments.Fri, Sep 25, 1:40 AM
clang/test/CodeGen/fp-floatcontrol-pragma.cpp
154

In this particular case we know for sure that the result does not depend on rounding mode and no FP exceptions occurs, so it is safe to constfold the expression.

Expressions like 1.0 / 0.0 or 1.0F + 0x0.000001p0F indeed may require execution in runtime, depending on the required exception handling. I would propose to connect their const-evaluability with FPExceptionMode and set the latter to strict whenever AllowFEnvAccess is set to true.

clang/test/CodeGen/pragma-fenv_access.c
10

Shall the rounding mode be dynamic rather than tonearest? If #pragma STDC FENV_ACCESS ON is specified, it means FP environment may be changed in arbitrary way and we cannot expect any particular rounding mode. Probably the environment was changed in the caller of func_01.

@sepavloff Is it OK if I continue work on this item? Not sure about the protocol when continuing someone else's patch.

It is OK for me. There is also an action in Phabricator "Commandeer Revision" to transfer ownership on a revision item.

I don't think however that the implementation in frontend is the main obstacle for enabling the pragma. It is the part of the standard and is user visible, so clang must provide satisfactory support so that users could try this feature in real applications. This support mainly depends on the support of constrained intrinsics in IR and codegen.

One of the probable ways to confirm the support is to build some pretty large project that uses floating point operations extensively, build it with option -fp-model=strict and check if it works. A good choice could be SPEC benchmarks. It would provide us with not only evidence of support but also with number how strict operations slow down execution. Maybe other projects may be used for this purpose, but I don't know such.

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

mibintc added inline comments.Fri, Sep 25, 9:33 AM
clang/test/CodeGen/fp-floatcontrol-pragma.cpp
154

Thank you, I will study these remarks. BTW I had inserted checks to verify semantic rules about the use of pragma float_control and fenv_access, to follow what Microsoft describes on this page, https://docs.microsoft.com/en-us/cpp/preprocessor/fenv-access?view=vs-2019
Quoting:
There are restrictions on the ways you can use the fenv_access pragma in combination with other floating-point settings:

You can't enable fenv_access unless precise semantics are enabled. Precise semantics can be enabled either by the float_control pragma, or by using the /fp:precise or /fp:strict compiler options. The compiler defaults to /fp:precise if no other floating-point command-line option is specified.

You can't use float_control to disable precise semantics when fenv_access(on) is set.

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

Thank you for trying this.

The error happens because static variable initializer gets rounding mode calculated from command-line options (dynamic), but this is wrong because such initializers must be calculated using constant rounding mode (tonearest in this case). Either we must force default FP mode in such initializers, or we are wrong when using FPOptions::defaultWithoutTrailingStorage. Need to analyze this problem more.

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

Thank you for trying this.

The error happens because static variable initializer gets rounding mode calculated from command-line options (dynamic), but this is wrong because such initializers must be calculated using constant rounding mode (tonearest in this case). Either we must force default FP mode in such initializers, or we are wrong when using FPOptions::defaultWithoutTrailingStorage. Need to analyze this problem more.

If the literal was typed, there was no diagnostic, "x = { 0.63F }". Thanks for the insight re: rounding mode.

rsmith added inline comments.Mon, Sep 28, 9:27 AM
clang/lib/AST/ExprConstant.cpp
2604

FFDiag

2836

Nit: do these checks in the opposite order; we don't need to compute FPOptions if the calculation was exact. (I assume we don't get an OK status if the result was rounded?)

2983

Change this function to take a BinaryOperator too rather than using a cast here.

4329

Change the E member to be a BinaryOperator* rather than using a cast here.

12491

We can form NaNs in constant evaluation using __builtin_nan(), and that's exposed to the user via std::numeric_limits, so I think we do need to check for this.

13481–13482

Yes, we should check for anything that could behave differently in a different FP environment.

13565
13623
13660

I don't think this works in general. There is no "wideness" order between PPC double-double and IEEE float128.

13684

Can we check whether the value is representable, not only the type? Eg, it would seem preferable to accept float x = 1.0;

13685

It would be useful to say which C standard and which footnote, eg "C11 footnote 123"

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

Thank you for trying this.

The error happens because static variable initializer gets rounding mode calculated from command-line options (dynamic), but this is wrong because such initializers must be calculated using constant rounding mode (tonearest in this case). Either we must force default FP mode in such initializers, or we are wrong when using FPOptions::defaultWithoutTrailingStorage. Need to analyze this problem more.

@sepavloff Please provide precise reference to support the claim "but this is wrong because such initializers must be calculated using constant rounding mode..." many thanks!

I tried using the 0924 version of the patch on an internal workload SPEC "cpu2017" and found that a few files failed to compile because of an error message on static initializer, like this: struct s { float f; }; static struct s x = {0.63}; Compiled with ffp-model=strict "initializer..is not a compile-time constant"

Thank you for trying this.

The error happens because static variable initializer gets rounding mode calculated from command-line options (dynamic), but this is wrong because such initializers must be calculated using constant rounding mode (tonearest in this case). Either we must force default FP mode in such initializers, or we are wrong when using FPOptions::defaultWithoutTrailingStorage. Need to analyze this problem more.

@sepavloff Please provide precise reference to support the claim "but this is wrong because such initializers must be calculated using constant rounding mode..." many thanks!

From http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2478.pdf:

6.7.9 Initialization

4 All the expressions in an initializer for an object that has static or thread storage duration shall be
constant expressions or string literals.

6.6 Constant expressions

2 A constant expression can be evaluated during translation rather than runtime, and accordingly
may be used in any place that a constant may be.

F.8.2 Translation
1 During translation, constant rounding direction modes (7.6.2) are in effect where specified. Elsewhere,
during translation the IEC 60559 default modes are in effect:

  • The rounding direction mode is rounding to nearest.

7.6.2 The FENV_ROUND pragma

2 The FENV_ROUND pragma provides a means to specify a constant rounding direction for floating point
operations for standard floating types within a translation unit or compound statement. …

Richard and I had a long conversation about this further up-thread, if you missed it.

mibintc updated this revision to Diff 295880.Fri, Oct 2, 11:30 AM

I patched my workspace with D88498 from Sept 29. I fixed some nits and also enabled FENV_ACCESS when the command line option specifies frounding-math. This corresponds to Microsoft's documentation of their option /fp:strict https://docs.microsoft.com/en-us/cpp/preprocessor/fenv-access?view=vs-2019 "The [/fp:strict] command-line option automatically enables fenv_access." Also if #pragma to enable fenv_access is seen, RoundingMode is set to Dynamic
I have some questions I'll add them as inline comments

sepavloff added inline comments.Mon, Oct 5, 12:25 AM
clang/include/clang/Basic/LangOptions.h
410–411

I don't think this deduction is correct.

AllowFEnvAccess implies that user can access FP environment in a way unknown to the compiler, for example by calling external functions or by using inline assembler. So if AllowFEnvAccess is set, the compiler must assume that rounding mode is dynamic, as user can set it to any value unknown to the compiler. Similarly, FPExceptionMode must be set to strict as user may read/write exception bits in any way or may associate them with traps. These deductions are valid and compiler must apply them.

The reverse is not valid. AllowFEnvAccess is a very coarse instrument to manipulate FP environment, it prevents from many optimization techniques. In contrast, setting RoundingMode to dynamic means only that rounding mode may be different from the default value. If a user changes rounding mode by calls to external functions like fesetmode or by using some intrinsic, the compiler still can, for example, reorder FP operations, if FPExceptionMode is ignore. Impact of performance in this case can be minimized.

I neglected to hit the "submit" button last week, these are the inline questions I have.

Also in addition to the comments above, check-clang is showing 1 LIT test failing AST/const-fpfeatures.cpp
I believe the clang constant folder is returning False for the expression, and the constant folding is being done by the LLVM constant folder
llvm-project/clang/test/AST/const-fpfeatures.cpp:21:11: error: CHECK: expected string not found in input
// CHECK: @V1 = {{.*}} float 1.000000e+00

^

<stdin>:1:1: note: scanning from here
; ModuleID = '/export/iusers/mblower/sandbox/llorg/llvm-project/clang/test/AST/const-fpfeatures.cpp'
^
<stdin>:24:1: note: possible intended match here
@V1 = global float 0.000000e+00, align 4

The reason i think clang constant folder/my patch/ isn't to blame for faulty logic is because if I change the test to make V1 constexpr, there is an error message "not constant". So i'm not sure if the bug is in clang or the test case needs to be fixed?

clang/lib/AST/ExprConstant.cpp
2840

This should be FFDiag?

12502

My scribbled notes say "ISO 10967 LIA-1
Equality returns invalid if either operand is signaling NaN
Other comparisons < <= >= > return invalid if either operand is NaN". I'm not putting my hands on the exact reference.

clang/test/CodeGen/pragma-fenv_access.c
10

Yes thanks @sepavloff ! i made this change

mibintc added inline comments.Mon, Oct 5, 8:37 AM
clang/include/clang/Basic/LangOptions.h
410–411

Thanks @sepavloff I'll make the change you recommend

mibintc updated this revision to Diff 296221.Mon, Oct 5, 10:34 AM

In the previous version of this patch, I enabled FENV_ACCESS if frounding-math, but @sepavloff commented that this deduction is not correct. I changed the logic to check for the "ffp-model=strict" settings, and if those settings are in effect from the command line, then I also enable fenv_access. To repeat what I've said elsewhere, this is the documented behavior of the "/fp:strict" Microsoft command line option.

For the LIT test clang/test/AST/const-fpfeatures.cpp, currently failing in this patch, the variables V1 and others are initialized via call to "global var init" which performs the rounding at execution time, I think that's correct not certain.

For the LIT test clang/test/AST/const-fpfeatures.cpp, currently failing in this patch, the variables V1 and others are initialized via call to "global var init" which performs the rounding at execution time, I think that's correct not certain.

This file does not contain pragma STDC FENV_ACCESS. As for using pragma STDC FENV_ROUND in constexpr functions, I don't see any reason to prohibit it.

Could it be a result of setting rounding mode to dynamic?

mibintc updated this revision to Diff 296520.Tue, Oct 6, 12:36 PM

I added a sentence in the clang UserManual pointing out that ffp-model=strict automatically enables FENV_ACCESS. I changed checkFloatingPointResult the way @sepavloff suggested to solve the LIT failure in clang/test/AST/const-fpfeatures.cpp, as long as the ExceptionBehavior is Ignore, the FENV_ACCESS is Off and RoundingMode isn't Dynamic, it's OK to fold even if the Status isn't OK. Now check-clang is running clean.

mibintc updated this revision to Diff 297260.Fri, Oct 9, 9:42 AM

I rebased the patch
I added documentation to UserManual showing that -ffp-model=strict enables FENV_ACCESS
I removed ActOnAfterCompoundStatementLeadingPragmas since it was redundant with work being done in ActOnCompoundStmt

This patch is based on D69272, perhaps i should merge both patches and upload them into this single review?

In D88498 @rsmith wrote,
s far from clear to me that this is correct in C++. In principle, for a dynamic initializer, the rounding mode could have been set by an earlier initializer.

Perhaps we can make an argument that, due to the permission to interleave initializers from different TUs, every dynamic initializer must leave the program in the default rounding mode, but I don't think even that makes this approach correct, because an initializer could do this:

double d;
double e = (fesetround(...), d = some calculation, fesetround(...default...), d);
I think we can only do this in C and will need something different for C++.

(I think this also has problems in C++ due to constexpr functions: it's not the case that all floating point operations that will be evaluated as part of the initializer lexically appear within it.)

For the initialization expression to be correct, the RoundingMode at the declaration of e needs to be set to Dynamic, right?

The GCC docs seem pretty clear that -frounding-math is meant to allow dynamic access to the FP environment. There might be optimizations that are perfectly legal with a constant but non-default rounding mode, and there might be some flag that only implies that weaker constraint, but -frounding-math needs to turn FENV_ACCESS on by default.

FYI: I posted a patch to implement the "manifestly constant evaluated" rule as https://reviews.llvm.org/D89360. It looks like the stricter rule doesn't work in practice; far too much C++ code uses -frounding-math and expects to be able to still include things like standard library headers that define constexpr functions and variables that involve floating-point rounding.

rsmith added inline comments.Fri, Oct 16, 3:41 PM
clang/lib/AST/ExprConstant.cpp
2548

I don't think we need the CPlusPlus check here; we bail out of this function in constant contexts in all language modes, and in non-constant contexts we want to abort evaluation under this condition in C too.

2600–2607

Following D89360, we should skip this check if Info.InConstantContext.

12500

Skip this check if Info.InConstantContext.

mibintc updated this revision to Diff 299395.Tue, Oct 20, 9:56 AM

I added StrictFPAttr clang function attribute accroding to @rjmccall 's suggestion instead of using a boolean on the function decl, rebased, and responded to other code review comments. will add replies inline

mibintc marked 2 inline comments as done.Tue, Oct 20, 10:20 AM

@rsmith asked "I don't see changes to the constant evaluator". The semantic rules for enabling this pragma require that strict or precise semantics be in effect., see SemaAttr.cpp And the floating point constant evaluation doesn't occur when strict or precise

What do you mean "the floating point constant evaluation doesn't occur when strict or precise"? I don't see any code to do that in ExprConstant.cpp, neither in trunk nor in this patch. The constant evaluator certainly is still invoked when we're in strict or precise mode. I would expect the evaluator would need to check for FP strictness whenever it encounters a floating-point operator (eg, here: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ExprConstant.cpp#L13340), and treat such cases as non-constant.

1.0/3.0 is now static folded. @rsmith Did you change your mind about this? Is static folding what you want here?

I'd like to see a C++ test for something like this:

float myAdd() {
#pragma STDC FENV_ACCESS ON
    static double v = 1.0 / 3.0;
    return v;
}

If that generates a runtime guarded initialization for v, then I'd be a bit more convinced we're getting this right.

Some inline comments, still owe you a list of test that fails. check-clang runs clean with this patch

clang/lib/AST/ExprConstant.cpp
2600–2607

I tried making this change but I got about 30 lit fails, maybe my patch was incorrect. I'll rerun with the patch and post the list of fails.

clang/test/CodeGen/fp-floatcontrol-pragma.cpp
159

@rsmith - the division is constant folded. is that right?

clang/test/Parser/pragma-fenv_access.c
29

@rsmith no diagnostic, is this OK?

mibintc updated this revision to Diff 299426.Tue, Oct 20, 11:22 AM

I made the change requested by @rsmith to HandleIntToFloatCast, avoiding the check if Info.InConstantContext, then I corrected 2 tests so that -verify matches.

clang/test/CXX/expr/expr.const/p2-0x.cpp
clang/test/Parser/pragma-fenv_access.c

(I had 30 lit tests failing in my sandbox but I fixed the logic, so please ignore that remark from my previous revision)
check-clang is running clean

rsmith accepted this revision.Tue, Oct 20, 3:57 PM

Thanks, I'm happy with this, though please wait a day or two for comments from the other reviewers.

We should also add some documentation covering the rules we're using for the floating-point environment in C++, particularly around initializers for variables with static storage duration, given that that's not governed by any language standard.

clang/test/CodeGen/fp-floatcontrol-pragma.cpp
159

Yes, that's in line with our new expectations for C++. Specifically: for static / thread-local variables, first try evaluating the initializer in a constant context, including in the constant floating point environment (just like in C), and then, if that fails, fall back to emitting runtime code to perform the initialization (which might in general be in a different FP environment). Given that 1.0 / 3.0 is a constant initializer, it should get folded using the constant rounding mode. But, for example:

double f() {
  int n = 0;
  static double v = 1.0 / 3.0 + 0 * n;
  return v;
}

... should emit a constrained fdiv at runtime, because the read of n causes the initializer to not be a constant initializer. And similarly:

double f() {
  double v = 1.0 / 3.0 + 0 * n;
  return v;
}

... should emit a constrained fdiv, because v is not a static storage duration variable, so there is formally no "first try evaluating the initializer as a constant" step.

clang/test/Parser/pragma-fenv_access.c
29

Under the new approach from D89360, I would expect no diagnostic with CONST defined as constexpr, because the initializer is then manifestly constant-evaluated, so should be evaluated in the constant rounding mode.

With CONST defined as merely const, I'd expect that we emit a constrained floating-point operation using the runtime rounding mode here. You can test that we don't constant-evaluate the value of a (non-constexpr) const float variable by using this hack (with CONST defined as const):

enum {
  e1 = (int)one, e3 = (int)three, e4 = (int)four, e_four_quarters = (int)(frac_ok * 4)
};
static_assert(e1 == 1  && e3 == 3 && e4 == 4 && e_four_quarters == 1, "");
enum {
  e_three_thirds = (int)(frac * 3) // expected-error {{not an integral constant expression}}
};

(The hack here is that we permit constant folding in the values of enumerators, and we allow constant folding to look at the evaluated values of const float variables. This should not be allowed for frac, because attempting to evaluate its initializer should fail.)

This revision is now accepted and ready to land.Tue, Oct 20, 3:57 PM
sepavloff accepted this revision.Tue, Oct 20, 11:28 PM

LGTM

There is concern with restoring FP options but it could be solved separately.

Looking forward to results of SPEC runs!

clang/docs/UsersManual.rst
1389

For a user manual Enables STDC FENV_ACCESS looks too vague. Probably we need to reword it to make more clear for compiler users. Something like Assumes implicit #pragma STDC FENV_ACCESS ON at the beginning of source files. Or, maybe this statement could be dropped.

clang/lib/Sema/SemaAttr.cpp
1020–1023

The previous values of rounding mode and exception behavior may differ from the default values. For example, #pragma STDC FENV_ROUND may set constant rounding direction different from FE_TONEAREST`. Similarly, command line options may set exception behavior different from ignore.
Can pragma stack be used for this?

clang/lib/Sema/SemaDecl.cpp
14281–14282

It is better to put assert into compound statement to avoid compiler confusion in release mode. Or to use something like: assert(!FSI->UsesFPIntrin || FD).