[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 2 inline comments as done.
mibintc added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2683
   }
+  if (const BinaryOperator *BE = dyn_cast<BinaryOperator>(E)) {
+    if (BE->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
----------------
rsmith wrote:
> `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*`.
OK I changed it to use BinaryOperator


================
Comment at: clang/lib/AST/ExprConstant.cpp:2685
+    if (BE->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
+      Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+      return false;
----------------
rsmith wrote:
> 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.
OK I made this change


================
Comment at: clang/lib/AST/ExprConstant.cpp:13283-13286
+    if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
+      Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+      return false;
+    }
----------------
rsmith wrote:
> Hm, does `fabs` depend on the floating-point environment? Is the concern the interaction with signaling NaNs?
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


================
Comment at: clang/lib/AST/ExprConstant.cpp:13372-13376
+  if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
+    // In C lang ref, footnote, cast may raise inexact exception.
+    Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+    return false;
+  }
----------------
rsmith wrote:
> 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?).
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? 


================
Comment at: clang/test/CodeGen/pragma-fenv_access.c:1
+// xxx: %clang_cc1 -ffp-exception-behavior=strict -frounding-math -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffp-exception-behavior=strict -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck %s
----------------
rsmith wrote:
> Is this RUN line intentionally disabled?
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


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

https://reviews.llvm.org/D87528