The change implements evaluation of constant floating point expressions
under non-default rounding modes. The main objective was to support
evaluation of global variable initializers, where constant rounding mode
may be specified by #pragma STDC FENV_ROUND.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2470 | I think the options really need to be passed in or else correctness is somewhat doomed here. For example, the call to CompoundAssignSubobjectHandler needs to propagate this down from the operator expression. | |
2705 | Feels like you should have a helper function like getActiveRoundingMode(Info, FPFeatures) | |
2734 | Is this processing something you can extract meaningfully into a common function that works with an opStatus, so that you can just write if (!checkFloatingPointFailure(Info, E, status, fpFeatures)) return false; in all these places? |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2470 | It is guaranteed by the way AST is built, no? As FP options may be changed only by pragmas and the pragmas can be specified only at file or block level, all sub-expression are evaluated at the same options. | |
2705 | It required implementation of statically polymorphic method Expr::getFPFeaturesInEffect but finally code looks better. | |
2734 | Good idea, thank you. I used a bit different name to have positive semantics. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2462 | Thanks, these look good. | |
2470 | Yes, but you can't actually reliably recover those settings from E unless you're sure E is one of a few kinds of expression. The concern is that E might end up being some closely-related expression that isn't actually the expression that carries the current settings, and then we'll fall back on using the global defaults. It's much more correct-by-construction to pass the settings down from the caller based on the caller's static knowledge of which expression is under consideration, and I think you'll see that that's pretty straightforward in practice. |
Updated patch
- improved Expr::getFPFeaturesInEffect,
- added tests for _Complex,
- added tests for constexpr,
- added chech in HandleFloatToFloatCast,
- implemented printing FPOptions in CompoundAssignOperator.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2470 | This is a peculiarity of CompoundAssignOperator, which itself makes conversion, without CastExpr. I added assert to ensure that other nodes cannot appear here. Also some tests with conversion in CompoundAssignOperator were added. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2470 | Alright, well, I don't understand the reluctance to pass this down, but I won't insist. LGTM. |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2438 | If the result is this false, shall this routine create a Note diag explaining the false result? |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2438 | It makes sense. Added respective diagnostics. |
Hi! It looks like this is causing a test failure on our aach64 builders (https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8868095822628984976?):
[1113/1114] Running the Clang regression tests llvm-lit: /b/s/w/ir/k/staging/llvm_build/bin/../../../llvm-project/llvm/utils/lit/lit/llvm/config.py:379: note: using clang: /b/s/w/ir/k/staging/llvm_build/bin/clang -- Testing: 26708 tests, 256 workers -- Testing: FAIL: Clang :: AST/const-fpfeatures-diag.c (269 of 26708) ******************** TEST 'Clang :: AST/const-fpfeatures-diag.c' FAILED ******************** Script: -- : 'RUN: at line 1'; /b/s/w/ir/k/staging/llvm_build/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/staging/llvm_build/lib/clang/12.0.0/include -nostdsysteminc -verify -ffp-exception-behavior=strict -Wno-unknown-pragmas /b/s/w/ir/k/llvm-project/clang/test/AST/const-fpfeatures-diag.c -- Exit Code: 1 Command Output (stderr): -- error: 'error' diagnostics expected but not seen: File /b/s/w/ir/k/llvm-project/clang/test/AST/const-fpfeatures-diag.c Line 8: initializer element is not a compile-time constant error: 'warning' diagnostics seen but not expected: (frontend): overriding currently unsupported use of floating point exceptions on this target 2 errors generated. -- ******************** Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. ******************** Failed Tests (1): Clang :: AST/const-fpfeatures-diag.c
Would you mind taking a look and sending out a fix? Thanks.
Hi!
This issue must be fixed by: https://reviews.llvm.org/rGf91b9c0f9858
Do you have recent changes from the trunk?
Thanks,
--Serge
Thanks for looking into it. We have that commit but it still seems to be
failing for us with the same error.
Commented in that review, but that patch has the wrong fix: it's based on the targets LLVM is configured with rather than the current test target.
In D88498 this test is removed because use of rounding mode changed. So you could just remove this test to fix the bot.
Can we revert this change and reland it later with the fix? It's been 3 days now and our bots are still red.
The change https://reviews.llvm.org/rG4e4f926e83cf removes the problematic
test.
Sorry for troubles.
Thanks,
--Serge
I noticed that this commit breaks MUSL 1.2.0. Here is an isolated test-case that illustrates the issue:
If the result is this false, shall this routine create a Note diag explaining the false result?