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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
