Page MenuHomePhabricator

[FPEnv] Evaluate constant expressions under non-default rounding modes
ClosedPublic

Authored by sepavloff on Sep 17 2020, 3:25 AM.

Details

Summary

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.

Diff Detail

Event Timeline

sepavloff created this revision.Sep 17 2020, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 3:25 AM
sepavloff requested review of this revision.Sep 17 2020, 3:25 AM
rjmccall added inline comments.Sep 17 2020, 10:56 PM
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?

Updated patch

sepavloff marked an inline comment as done.Sep 18 2020, 10:54 AM
sepavloff added inline comments.
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.

rjmccall added inline comments.Sep 18 2020, 11:13 AM
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.

sepavloff updated this revision to Diff 293974.Sep 24 2020, 1:59 AM
sepavloff marked an inline comment as done.

Updated patch

  • improved Expr::getFPFeaturesInEffect,
  • added tests for _Complex,
  • added tests for constexpr,
  • added chech in HandleFloatToFloatCast,
  • implemented printing FPOptions in CompoundAssignOperator.
sepavloff added inline comments.Sep 24 2020, 2:04 AM
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.

rjmccall accepted this revision.Sep 24 2020, 10:39 AM
rjmccall added inline comments.
clang/lib/AST/ExprConstant.cpp
2470

Alright, well, I don't understand the reluctance to pass this down, but I won't insist. LGTM.

This revision is now accepted and ready to land.Sep 24 2020, 10:39 AM
mibintc added inline comments.Sep 24 2020, 10:58 AM
clang/lib/AST/ExprConstant.cpp
2438

If the result is this false, shall this routine create a Note diag explaining the false result?

Added diagnostic message

sepavloff added inline comments.Sep 24 2020, 10:42 PM
clang/lib/AST/ExprConstant.cpp
2438

It makes sense. Added respective diagnostics.

mibintc accepted this revision.Sep 25 2020, 8:02 AM
This revision was landed with ongoing or failed builds.Sep 26 2020, 4:57 AM
This revision was automatically updated to reflect the committed changes.

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.

Thanks for looking into it. We have that commit but it still seems to be
failing for us with the same error.

In D88498 this test is removed because use of rounding mode changed. So you could just remove this test to fix the bot.

phosek added a subscriber: phosek.Tue, Sep 29, 12:55 PM

Thanks for looking into it. We have that commit but it still seems to be
failing for us with the same error.

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:

https://godbolt.org/z/6bdnEh