[FPEnv] Apply dynamic rounding to function body only

[sepavloff]

Summary

Support of rounding mode in constant evaluation resulted in some
number of compile errors for constructs that previously were compiled
successfully. For example, the following code starts failing at
compilation if FP exception behavior is set to strict:

struct S { float f; };
static struct S x = {0.63};

This happenes because setting strict behavior sets rounding mode to
dymanic. In this case the value of initializer depends on the current
rounding mode and cannot be evaluated in compile time.

Using dynamic as rounding mode make no sense outside function bodies.
For example, even if the initializer is evaluated dynamically, this happens
before the execution of main, so there is no possibility to set dynamic
rounding mode for it. C requires that initializers are evaluated using
constant rounding mode. It makes sense to apply this rule to C++ as well.

With this change dynamic rounding mode is applied to function bodies. In
other cases, like evaluation of initializers the constant rounding mode
is applied, which is 'tonearest' unless it was changed by '#pragma STDC
FENV_ROUND'.

[mibintc]

thanks for this!

[rsmith]

clang/lib/Parse/ParseDecl.cpp

2290

It'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.)

[mibintc]

clang/test/AST/const-fpfeatures-strict.c

39

I think the initializer for var_04 is evaluated at translation time, therefore the initialization expression would look the same as var_01 above, using default rounding mode. Same for var_05 and var_06. They have "static duration"

[sepavloff]

Updated patch

[sepavloff]

clang/lib/Parse/ParseDecl.cpp

2290

I changed the code to confine it with C and constexpr only. Hopefully this would be enough to enable build of SPECS attempted by @mibintc (https://reviews.llvm.org/D87528#2295015). However in long-term perspective we should return to this code.

The intent was to align behavior of C and C++. If an initializer is valid in C, then it should produce the same code in C++. If the source code like

float appx_coeffs_fp32[3 * 256] = {
    SEGMENT_BIAS + 1.4426950216,
…

produces compact table in C mode and huge initialization code in C++, it would be strange from user viewpoint and would not give any advantage.

C in C2x presents pretty consistent model, provided that #pragma STDC FENV_ROUND FE_DYNAMIC does not set constant rounding mode. Initializers for variables with static allocation are always evaluated in constant rounding mode and user can chose the mode using pragma FENV_ROUND.

When extending this model to C++ we must solve the problem of dynamic initialization. It obviously occurs in runtime rounding mode, so changing between static and dynamic initialization may change semantics. If however dynamic initialization of global variable occurs in constant rounding mode (changing FP control modes in initializers without restoring them is UB), static and dynamic initialization would be semantically equivalent.

We cannot apply the same rule to local static variables, as they are treated differently in C and C++. So the code:

float func_01(float x) {
  #pragma STDC FENV_ACCESS ON
  static float val = 1.0/3.0;
  return val;
}

Would be compiled differently in C and C++.

[sepavloff]

clang/test/AST/const-fpfeatures-strict.c

39

The statement #pragma STDC FENV_ROUND FE_UPWARD changes constant rounding mode, so evaluation of the initializer for var_04 produces different result than in the case of var_01, which indeed is evaluated using default rounding mode.

[sepavloff]

clang/lib/Parse/ParseDecl.cpp

2290

Additional note:

If initialization is dynamic and constant rounding mode is not default, the body of initializer is executed with dynamic rounding mode set to the constant mode. That is, the code:

#pragma STDC FENV_ROUND FE_UPWARD
float var = some_init_expr;

generates code similar to:

float var = []()->float {
  #pragma STDC FENV_ROUND FE_UPWARD
  return some_init_expr;
}

So initializers of global variable must conform to:

1. If the initialization is dynamic and dynamic rounding mode is changed in the initializer, it must be restored upon finishing the initializer.
2. The initializer is evaluated in constant rounding mode. If the initialization is dynamic, the initializing code is executed in dynamic rounding mode set to the constant rounding mode.

These seems enough to provide compatibility with C and the same semantics for static and dynamic initialization.

[mibintc]

ping!

clang/lib/Parse/ParseDecl.cpp

2290

@rsmith Serge changed the patch to confine it with C and constexpr only, is that adequate to move forward with this patch, and we can return to C++ at some point down the road?

[rsmith]

clang/lib/Parse/ParseDecl.cpp

2290

This still seems inappropriate for the constexpr case -- we shouldn't have different behavior based on whether an expression appears directly in the initializer or in a constexpr function invoked by that initializer. (It also violates the C++ standard's recommendation that floating-point evaluation during translation and at runtime produce the same value.) Please see the discussion in D87528. Let's continue the conversation there; we shouldn't be splitting this discussion across two separate review threads.

[sepavloff]

Updated patch

• Reverted check to the previous version, in which it applied to C++ file level variables also.
• Added workaround for constexpr functions. Now they are parsed with constant rounding mode, which allows to use them with option -frounding-math.

Setting call-site rounding mode is not implemented yet.

[rsmith]

In D88498#2329845, @sepavloff wrote:

• Reverted check to the previous version, in which it applied to C++ file level variables also.

This presumably reintroduces the misbehavior for

double d;
double e = (fesetround(...), d = some calculation, fesetround(...default...), d);

in which some calculation will be treated as being in the default rounding mode, right?

• Added workaround for constexpr functions. Now they are parsed with constant rounding mode, which allows to use them with option -frounding-math.

This is inappropriate. When a constexpr function is invoked at runtime, it should behave exactly like any other function. Marking a function as constexpr should not cause it to round differently when used outside of constant expressions.

[sepavloff]

Remade the patch

[sepavloff]

Actually this solution also behaves wrong in some cases.

[rsmith]

The tests in this patch exhibit the same behavior with and without the patch applied; I think almost all the functionality changes from here are superseded by the change to consider whether we're in a manifestly constant evaluated context. As far as I can tell, it only affects the behavior of C++ dynamic initializers in FENV_ACCESS ON regions by making calls to feset* be undefined behavior. I'm unconvinced that's the right way to extend the behavior of the FENV_* functionality to C++. Consider this example:

#include <fenv.h>
#pragma STDC FENV_ACCESS ON
struct InRoundingMode {
  int mode;
  int oldmode = fegetround();
  int ok1 = fesetround(mode);
  double value;
  int ok2 = fesetround(oldmode);
};
double d1 = InRoundingMode{.mode = FE_UPWARD, .value = 1.0 / 3.0}.value;
double d2 = InRoundingMode{.mode = FE_DOWNWARD, .value = 1.0 / 3.0}.value;

I don't think this is unreasonable: this code changes the rounding mode, performs a calculation, and then changes it back, all within a dynamic initializer, and all in an FENV_ACCESS ON region. I think it would be unreasonable to say that the FENV_ACCESS doesn't apply to the initializer, and the initializer therefore has undefined behavior.

So my inclination is to say that the status quo (prior to this patch) is preferable behavior. The new tests look valuable.

clang/test/CodeGenCXX/rounding-math.cpp

11 ↗

(On Diff #298375)

As discussed in other review threads, this should be a constant initializer with value 1.0; indeed, that's what we get both with and without the code change from this patch (this test fails when the patch is applied to Clang trunk for this reason).

clang/test/SemaCXX/rounding-math-diag.cpp

7 ↗

(On Diff #298375)

This is not the diagnostic we give with this patch applied (applying this patch results in this test failing for me): we say "read of non-constexpr variable is not allowed in a constant expression". (We don't consider the initializer of the variable.)

clang/test/SemaCXX/rounding-math.cpp

1 ↗

(On Diff #298375)

This patch needs to be rebased; a test file with this name already exists.

[sepavloff]

In D88498#2339630, @rsmith wrote:

The tests in this patch exhibit the same behavior with and without the patch applied; I think almost all the functionality changes from here are superseded by the change to consider whether we're in a manifestly constant evaluated context.

Thank you for review and explanations!
So I am abandoning the patch.

So my inclination is to say that the status quo (prior to this patch) is preferable behavior. The new tests look valuable.

I'll try to extract the tests to a new review item.

clang/include/clang/Basic/LangOptions.h

public:
 
   storage_type getAsOpaqueInt() const { return Value; }
   static FPOptions getFromOpaqueInt(storage_type Value) {
     FPOptions Opts;
     Opts.Value = Value;
     return Opts;
   }
 
+  storage_type getDiffWith(FPOptions FPO) const {
+    storage_type Diff = 0;
+#define OPTION(NAME, TYPE, WIDTH, PREVIOUS)                                    \
+  if (get##NAME() != FPO.get##NAME())                                          \
+    Diff |= NAME##Mask;
+#include "clang/Basic/FPOptions.def"
+    return Diff;
+  }
+
   // We can define most of the accessors automatically:
 #define OPTION(NAME, TYPE, WIDTH, PREVIOUS)                                    \
   TYPE get##NAME() const {                                                     \
     return static_cast<TYPE>((Value & NAME##Mask) >> NAME##Shift);             \
   }                                                                            \
   void set##NAME(TYPE value) {                                                 \
     Value = (Value & ~NAME##Mask) | (storage_type(value) << NAME##Shift);      \
   }
public:
   static constexpr storage_type OverrideMaskBits =
       (static_cast<storage_type>(1) << FPOptions::StorageBitSize) - 1;
 
   FPOptionsOverride() {}
   FPOptionsOverride(const LangOptions &LO)
       : Options(LO), OverrideMask(OverrideMaskBits) {}
   FPOptionsOverride(FPOptions FPO)
       : Options(FPO), OverrideMask(OverrideMaskBits) {}
+  FPOptionsOverride(FPOptions New, FPOptions Old)
+      : Options(New), OverrideMask(New.getDiffWith(Old)) {}
 
   bool requiresTrailingStorage() const { return OverrideMask != 0; }
 
   void setAllowFPContractWithinStatement() {
     setFPContractModeOverride(LangOptions::FPM_On);
   }
 
   void setAllowFPContractAcrossStatement() {

clang/include/clang/Sema/Sema.h

public:
   PragmaStack<StringLiteral *> DataSegStack;
   PragmaStack<StringLiteral *> BSSSegStack;
   PragmaStack<StringLiteral *> ConstSegStack;
   PragmaStack<StringLiteral *> CodeSegStack;
 
   // This stack tracks the current state of Sema.CurFPFeatures.
   PragmaStack<FPOptionsOverride> FpPragmaStack;
   FPOptionsOverride CurFPFeatureOverrides() {
-    FPOptionsOverride result;
-    if (!FpPragmaStack.hasValue()) {
-      result = FPOptionsOverride();
-    } else {
-      result = FpPragmaStack.CurrentValue;
-    }
-    return result;
+    return FpPragmaStack.CurrentValue;
   }
 
   // RAII object to push / pop sentinel slots for all MS #pragma stacks.
   // Actions should be performed only if we enter / exit a C++ method body.
   class PragmaStackSentinelRAII {
   public:
     PragmaStackSentinelRAII(Sema &S, StringRef SlotLabel, bool ShouldAct);
     ~PragmaStackSentinelRAII();
public:
   ~Sema();
 
   /// Perform initialization that occurs after the parser has been
   /// initialized but before it parses anything.
   void Initialize();
 
   const LangOptions &getLangOpts() const { return LangOpts; }
   OpenCLOptions &getOpenCLOptions() { return OpenCLFeatures; }
-  FPOptions     &getCurFPFeatures() { return CurFPFeatures; }
 
   DiagnosticsEngine &getDiagnostics() const { return Diags; }
   SourceManager &getSourceManager() const { return SourceMgr; }
   Preprocessor &getPreprocessor() const { return PP; }
   ASTContext &getASTContext() const { return Context; }
   ASTConsumer &getASTConsumer() const { return Consumer; }
   ASTMutationListener *getASTMutationListener() const;
   ExternalSemaSource* getExternalSource() const { return ExternalSource; }
 
+  const FPOptions &getCurFPFeatures() const { return CurFPFeatures; }
+  void setCurFPFeatures(FPOptions FPO) {
+    FpPragmaStack.CurrentValue = FPOptionsOverride(FPO, CurFPFeatures);
+    CurFPFeatures = FPO;
+  }
+
   ///Registers an external source. If an external source already exists,
   /// creates a multiplex external source and appends to it.
   ///
   ///\param[in] E - A non-null external sema source.
   ///
   void addExternalSource(ExternalSemaSource *E);
 
   void PrintStats() const;

clang/lib/Parse/ParseDecl.cpp

if (Tok.is(tok::kw_delete)) {
       if (Tok.is(tok::code_completion)) {
         Actions.CodeCompleteInitializer(getCurScope(), ThisDecl);
         Actions.FinalizeDeclaration(ThisDecl);
         cutOffParsing();
         return nullptr;
       }
 
       PreferredType.enterVariableInit(Tok.getLocation(), ThisDecl);
+
+      Sema::FPFeaturesStateRAII FPO(Actions);
+      if (auto VD = dyn_cast_or_null<VarDecl>(ThisDecl))
+        if (!VD->isInvalidDecl()) {
+          // If variable requires constant initialization, set constant
+          // rounding mode.
+          if (VD->isFileVarDecl() || VD->isConstexpr() ||
+              (!getLangOpts().CPlusPlus && VD->isStaticLocal())) {
+            if (Actions.getCurFPFeatures().getRoundingMode() ==
+                llvm::RoundingMode::Dynamic) {
+              // If constant rounding mode is 'dynamic', it means that a command
+              // line option line like `-ffpmodel=strict` is in effect. Set
+              // constant rounding to default in this case.
+              FPOptions NewFPO = Actions.getCurFPFeatures();
+              NewFPO.setRoundingMode(llvm::RoundingMode::NearestTiesToEven);
+              Actions.setCurFPFeatures(NewFPO);
+            }
+          }
+        }
+
       ExprResult Init = ParseInitializer();
 
       // If this is the only decl in (possibly) range based for statement,
       // our best guess is that the user meant ':' instead of '='.
       if (Tok.is(tok::r_paren) && FRI && D.isFirstDeclarator()) {
         Diag(EqualLoc, diag::err_single_decl_assign_in_for_range)
             << FixItHint::CreateReplacement(EqualLoc, ":");
         // We are trying to stop parser from looking for ';' in this for

clang/test/AST/const-fpfeatures-diag.c

-// RUN: %clang_cc1 -verify -ffp-exception-behavior=strict -Wno-unknown-pragmas %s
-
-// REQUIRES: x86-registered-target
-
-#pragma STDC FENV_ROUND FE_DYNAMIC
-
-// nextUp(1.F) == 0x1.000002p0F
-
-float F1 = 0x1.000000p0F + 0x0.000002p0F;
-float F2 = 0x1.000000p0F + 0x0.000001p0F; // expected-error{{initializer element is not a compile-time constant}}

clang/test/AST/const-fpfeatures-strict.c

+// RUN: %clang_cc1 -S -emit-llvm -ffp-exception-behavior=strict -Wno-unknown-pragmas %s -o - | FileCheck %s
+
+// nextUp(1.F) == 0x1.000002p0F
+
+struct S {
+  float f;
+};
+
+static struct S var_01 = {0x1.000001p0};
+struct S *func_01() {
+  return &var_01;
+}
+
+struct S var_02 = {0x1.000001p0};
+
+struct S *func_03() {
+  static struct S var_03 = {0x1.000001p0};
+  return &var_03;
+}
+
+// CHECK: @var_01 = {{.*}} %struct.S { float 1.000000e+00 }
+// CHECK: @var_02 = {{.*}} %struct.S { float 1.000000e+00 }
+// CHECK: @func_03.var_03 = {{.*}} %struct.S { float 1.000000e+00 }
+
+#pragma STDC FENV_ROUND FE_UPWARD
+
+static struct S var_04 = {0x1.000001p0};
+struct S *func_04() {
+  return &var_04;
+}
+
+struct S var_05 = {0x1.000001p0};
+
+struct S *func_06() {
+  static struct S var_06 = {0x1.000001p0};
+  return &var_06;
+}
+
+// CHECK: @var_04 = {{.*}} %struct.S { float 0x3FF0000020000000 }
+// CHECK: @var_05 = {{.*}} %struct.S { float 0x3FF0000020000000 }
+// CHECK: @func_06.var_06 = {{.*}} %struct.S { float 0x3FF0000020000000 }