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

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

rsmith 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()) {
----------------
`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*`.


================
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;
----------------
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.


================
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;
+    }
----------------
Hm, does `fabs` depend on the floating-point environment? Is the concern the interaction with signaling NaNs?


================
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;
+  }
----------------
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?).


================
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
----------------
Is this RUN line intentionally disabled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87528