Page MenuHomePhabricator

[Clang] Support constexpr builtin fmax
ClosedPublic

Authored by Izaron on Sep 21 2022, 8:57 AM.

Details

Summary

Support constexpr version of __builtin_fmax and its variations.

Diff Detail

Event Timeline

Izaron created this revision.Sep 21 2022, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2022, 8:57 AM
Izaron requested review of this revision.Sep 21 2022, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2022, 8:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This patch is derived from https://reviews.llvm.org/D134136 (which was abandoned due to poor tests and overcomplication)

The constexpr version of fmax matches the libc realization: the libc realization is here.

It is verified with test that matches libc test: the libc test is here.

If this patch is approved, I promise to do a similar patch for the fmin function 😀

I think __builtin_fmax can raise a floating-point exception; in that case, it wouldn't be constant, I think? Not sure how consistent we are about handling that sort of thing in constant evaluation at the moment.

I think __builtin_fmax can raise a floating-point exception; in that case, it wouldn't be constant, I think? Not sure how consistent we are about handling that sort of thing in constant evaluation at the moment.

https://eel.is/c++draft/library.c#3 says that a floating-point exception other than FE_INEXACT causes it to not be a constant expression, if I have constant expression wording right. FE_INVALID can be raised if the inputs are signalling NaNs, but even if we're claiming implementation of Annex F, it's okay to treat sNaN as qNaN unless we're claiming FE_SNANS_ALWAYS_SIGNAL (this is new in C2x, I think). We shouldn't be claiming that except maybe if we're using -fp-model=strict. I don't think #pragma STDC FENV_ACCESS ON bears any relevance here, but now I'm wondering what needs to happen with that for regular floating-point operations that may trigger exceptions.

clang/lib/AST/ExprConstant.cpp
14035–14036

If I'm reading APFloat's source correctly, this doesn't suffice to consistently make __builtin_fmax(±0.0, ±0.0) return +0.0 if one of the zeroes is positive. (We're not required to return +0.0 in this case, but most libm implementations seem to endeavor to return +0.0 in this situation, even if the LLVM intrinsic we lower to doesn't).

clang/test/Sema/constant-builtins-fmax.cpp
36–40

These tests aren't covering what the sign of zero is in these cases.

We shouldn't be claiming that except maybe if we're using -fp-model=strict

Makes sense.

I'm wondering what needs to happen with that for regular floating-point operations that may trigger exceptions

In general, floating-point exceptions are affected by whether we're semantically required to do constant evaluation (InConstantContext in the code). If we're evaluating something that has to be a constant expression, we ignore exceptions. Otherwise, we're doing opportunistic evaluation, so we have to honor the rounding mode specified by flags/pragmas.

Izaron added inline comments.Sep 28 2022, 4:45 PM
clang/lib/AST/ExprConstant.cpp
14035–14036

These cases are covered in tests (in a wrong manner though as I see), I was "checking" that we have +0.0 if one of the zeroes is positive

clang/test/Sema/constant-builtins-fmax.cpp
36–40

Cool! Thank you, today I learned that this compiles:

static_assert(0.0 == 0.0);
static_assert(0.0 == -0.0);
static_assert(0.0 == +0.0);

So I think I need to test the +0.0 cases with the __builtin_signbit (I didn't find another way), BUT this function is not constexpr yet. So I need to implement a constexpr __builtin_signbit first... In a new pull request.

Izaron updated this revision to Diff 463715.Sep 28 2022, 4:46 PM

Rebase and fix windows __float128 failure

efriedma added inline comments.Sep 29 2022, 9:53 AM
clang/test/Sema/constant-builtins-fmax.cpp
36–40

I think __builtin_copysign should be sufficient.

Izaron updated this revision to Diff 464990.Oct 4 2022, 6:13 AM

Fix corner case with pos/neg zeroes

Fix tests for pos/neg zeroes with __builtin_copysign

Thanks to @efriedma and @jcranmer-intel!

Izaron marked 3 inline comments as done.Oct 4 2022, 6:13 AM

Regarding Annex F requirements, do we document that we conform to Annex F and do we have any actual test coverage for it? This question has come up for me quite a bit as I've worked on our C status page and I've never found a satisfactory answer of what we intend to support.

https://eel.is/c++draft/library.c#3 says that a floating-point exception other than FE_INEXACT causes it to not be a constant expression, if I have constant expression wording right. FE_INVALID can be raised if the inputs are signalling NaNs, but even if we're claiming implementation of Annex F, it's okay to treat sNaN as qNaN unless we're claiming FE_SNANS_ALWAYS_SIGNAL (this is new in C2x, I think). We shouldn't be claiming that except maybe if we're using -fp-model=strict. I don't think #pragma STDC FENV_ACCESS ON bears any relevance here, but now I'm wondering what needs to happen with that for regular floating-point operations that may trigger exceptions.

I think FENV_ACCESS is relevant. [library.c]p3 says C++ constant expressions follow Annex F requirements, and C2x F.8.4p1 says that FENV_ACCESS impacts constant expression evaluation.

clang/lib/AST/ExprConstant.cpp
14036
Izaron updated this revision to Diff 465106.Oct 4 2022, 11:50 AM

Fix comment wording.

Thanks to @aaron.ballman!

Izaron marked an inline comment as done.Oct 4 2022, 11:50 AM
jcranmer-intel added inline comments.Oct 7 2022, 11:57 AM
clang/lib/AST/ExprConstant.cpp
14031–14032

I think you should add a comment saying // TODO: Handle sNaN, just so that we remember to revisit this later.

Izaron updated this revision to Diff 466151.Oct 7 2022, 12:08 PM

Add TODO comment about sNaN. Thanks to @jcranmer-intel!

Izaron marked an inline comment as done.Oct 7 2022, 12:09 PM
jcranmer-intel accepted this revision.Oct 7 2022, 12:21 PM
This revision is now accepted and ready to land.Oct 7 2022, 12:21 PM
This revision was automatically updated to reflect the committed changes.
Izaron added a comment.Oct 7 2022, 1:43 PM

I've got an email about "Buildbot failure" with this link: https://lab.llvm.org/buildbot/#/builders/216/builds/10955

Line 51: 'static_assert' with no message is a C++17 extension

Why is it failing if pre-merge checks has been passed? =(

You might need to explicitly specify -std=c++17 in the RUN line. (That bot has a different target triple, and I think with that triple the default C++ standard isn't C++17?)

You might need to explicitly specify -std=c++17 in the RUN line. (That bot has a different target triple, and I think with that triple the default C++ standard isn't C++17?)

Thanks @efriedma! Could you please take a look at my fix with added -std=c++17? https://reviews.llvm.org/D135486

Sorry for inconvenience, it was a sad surprise =(