[PATCH] D88498: [FPEnv] Apply dynamic rounding to function body only

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.
Herald added a subscriber: dexonsmith.

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.



================
Comment at: clang/test/CodeGenCXX/rounding-math.cpp:11
+
+// CHECK: @V1 = {{.*}}global float 0.000000e+00, align 4
+// CHECK: @V2 = {{.*}}global float 1.000000e+00, align 4
----------------
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).


================
Comment at: clang/test/SemaCXX/rounding-math-diag.cpp:7
+  static_assert(C1 == 1.0F, "");         // expected-error{{static_assert expression is not an integral constant expression}}
+                                         // expected-note at -1{{initializer of 'C1' is not a constant expression}}
+}
----------------
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.)


================
Comment at: clang/test/SemaCXX/rounding-math.cpp:1
+// RUN: %clang_cc1 -triple x86_64-linux -verify -frounding-math -Wno-unknown-pragmas %s
+// expected-no-diagnostics
----------------
This patch needs to be rebased; a test file with this name already exists.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88498/new/

https://reviews.llvm.org/D88498