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