[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:2446
+      fpOptions.isFPConstrained()) {
+    Info.CCEDiag(E, diag::note_constexpr_float_arithmetic_strict);
+    return false;
----------------
FFDiag


================
Comment at: clang/lib/AST/ExprConstant.cpp:2677
+  if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained() &&
+      llvm::APFloatBase::opOK != status) {
+    Info.FFDiag(E, diag::note_constexpr_float_arithmetic_strict);
----------------
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?)


================
Comment at: clang/lib/AST/ExprConstant.cpp:2827
 
-      if (!handleFloatFloatBinOp(Info, E, LHSFloat, Opcode,
-                                 RHSElt.getFloat())) {
+      if (!handleFloatFloatBinOp(Info, dyn_cast<BinaryOperator>(E), LHSFloat,
+                                 Opcode, RHSElt.getFloat())) {
----------------
Change this function to take a `BinaryOperator` too rather than using a cast here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4158
     } else if (RHS.isFloat()) {
+      const BinaryOperator *BE = dyn_cast<BinaryOperator>(E);
+      const FPOptions fpOptions = BE->getFPFeaturesInEffect(
----------------
Change the `E` member to be a `BinaryOperator*` rather than using a cast here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:12242
+    // but this doesn't affect constant folding since NaN are
+    // not constant expressions.
+
----------------
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.


================
Comment at: clang/lib/AST/ExprConstant.cpp:13290
   case Builtin::BI__builtin_fabsf128:
+    // The c lang ref says that "fabs raises no floating-point exceptions,
+    // even if x is a signaling NaN. The returned value is independent of
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:13347
 bool FloatExprEvaluator::VisitUnaryOperator(const UnaryOperator *E) {
+  // In C lang ref, the unary - raises no floating point exceptions,
+  // even if the operand is signalling.
----------------



================
Comment at: clang/lib/AST/ExprConstant.cpp:13383
+  return ST && DT && DT->isRealFloatingType() && ST->isRealFloatingType() &&
+         ST->getKind() <= DT->getKind();
+}
----------------
I don't think this works in general. There is no "wideness" order between PPC double-double and IEEE float128.


================
Comment at: clang/lib/AST/ExprConstant.cpp:13406
+    if (E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained() &&
+        !isWideningFPConversion(E->getType(), SubExpr->getType())) {
+      // In C lang ref, footnote, cast may raise inexact exception.
----------------
Can we check whether the value is representable, not only the type? Eg, it would seem preferable to accept `float x = 1.0;`


================
Comment at: clang/lib/AST/ExprConstant.cpp:13407
+        !isWideningFPConversion(E->getType(), SubExpr->getType())) {
+      // In C lang ref, footnote, cast may raise inexact exception.
+      // Cast may also raise invalid.
----------------
It would be useful to say which C standard and which footnote, eg "C11 footnote 123"


================
Comment at: clang/lib/AST/ExprConstant.cpp:13197-13198
   assert(E->isRValue() && E->getType()->isRealFloatingType());
+  if (Info.isStrictFP)
+    return false;
   return FloatExprEvaluator(Info, Result).Visit(E);
----------------
mibintc wrote:
> rsmith wrote:
> > 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.
> > 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?
Yes, we should check for anything that could behave differently in a different FP environment.


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

https://reviews.llvm.org/D87528