Support constexpr version of __builtin_fmax and its variations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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 | ||
---|---|---|
14033–14034 | 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 | ||
35–39 | 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.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
14033–14034 | 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 | ||
35–39 | 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. |
clang/test/Sema/constant-builtins-fmax.cpp | ||
---|---|---|
35–39 | I think __builtin_copysign should be sufficient. |
Fix corner case with pos/neg zeroes
Fix tests for pos/neg zeroes with __builtin_copysign
Thanks to @efriedma and @jcranmer-intel!
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.
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 | ||
---|---|---|
14034 |
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
14029–14030 | I think you should add a comment saying // TODO: Handle sNaN, just so that we remember to revisit this later. |
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?)
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 =(
I think you should add a comment saying // TODO: Handle sNaN, just so that we remember to revisit this later.