-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[C++23] [CLANG] Adding C++23 constexpr math functions: fmin, fmax and frexp. #88978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-llvm-adt @llvm/pr-subscribers-clang Author: Zahira Ammarguellat (zahiraam) ChangesAdd support for for fmin and frexp. Full diff: https://github.com/llvm/llvm-project/pull/88978.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 52c0dd52c28b11..a35c77286229ff 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -3440,7 +3440,7 @@ def Fmod : FPMathTemplate, LibBuiltin<"math.h"> {
def Frexp : FPMathTemplate, LibBuiltin<"math.h"> {
let Spellings = ["frexp"];
- let Attributes = [NoThrow];
+ let Attributes = [NoThrow, Constexpr];
let Prototype = "T(T, int*)";
let AddBuiltinPrefixedAlias = 1;
}
@@ -3618,7 +3618,7 @@ def Fmax : FPMathTemplate, LibBuiltin<"math.h"> {
def Fmin : FPMathTemplate, LibBuiltin<"math.h"> {
let Spellings = ["fmin"];
- let Attributes = [NoThrow, Const];
+ let Attributes = [NoThrow, Const, Constexpr];
let Prototype = "T(T, T)";
let AddBuiltinPrefixedAlias = 1;
let OnlyBuiltinPrefixedAliasIsConstexpr = 1;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 5a36621dc5cce2..1a6abb386071c7 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2922,7 +2922,7 @@ static bool handleFloatFloatBinOp(EvalInfo &Info, const BinaryOperator *E,
// If during the evaluation of an expression, the result is not
// mathematically defined [...], the behavior is undefined.
// FIXME: C++ rules require us to not conform to IEEE 754 here.
- if (LHS.isNaN()) {
+ if (!Info.getLangOpts().CPlusPlus23 && LHS.isNaN()) {
Info.CCEDiag(E, diag::note_constexpr_float_arithmetic) << LHS.isNaN();
return Info.noteUndefinedBehavior();
}
@@ -14547,6 +14547,17 @@ bool FloatExprEvaluator::VisitCallExpr(const CallExpr *E) {
default:
return false;
+ case Builtin::BI__builtin_frexpf:
+ case Builtin::BI__builtin_frexp: {
+ LValue Pointer;
+ if (!EvaluateFloat(E->getArg(0), Result, Info) ||
+ !EvaluatePointer(E->getArg(1), Pointer, Info))
+ return false;
+ llvm::RoundingMode RM = getActiveRoundingMode(Info, E);
+ int FrexpExp;
+ Result = llvm::frexp(Result, FrexpExp, RM);
+ return true;
+ }
case Builtin::BI__builtin_huge_val:
case Builtin::BI__builtin_huge_valf:
case Builtin::BI__builtin_huge_vall:
@@ -14638,6 +14649,8 @@ bool FloatExprEvaluator::VisitCallExpr(const CallExpr *E) {
return true;
}
+ case Builtin::BIfmin:
+ case Builtin::BIfminf:
case Builtin::BI__builtin_fmin:
case Builtin::BI__builtin_fminf:
case Builtin::BI__builtin_fminl:
diff --git a/clang/test/CodeGenCXX/constexpr-math.cpp b/clang/test/CodeGenCXX/constexpr-math.cpp
new file mode 100644
index 00000000000000..ad7b6779a4ae0e
--- /dev/null
+++ b/clang/test/CodeGenCXX/constexpr-math.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -x c++ -triple x86_64-unknown-unknown -std=c++23 \
+// RUN: -DWIN -emit-llvm -o - %s | FileCheck %s --check-prefixes=WIN
+
+// RUN: %clang_cc1 -x c++ -triple x86_64-unknown-unknown -std=c++23 \
+// RUN: -emit-llvm -o - %s | FileCheck %s --check-prefixes=LNX
+
+#ifdef WIN
+#define INFINITY ((float)(1e+300 * 1e+300))
+#define NAN (-(float)(INFINITY * 0.0F))
+#else
+#define NAN (__builtin_nanf(""))
+#define INFINITY (__builtin_inff())
+#endif
+
+int func()
+{
+ int i;
+
+ // fmin
+ constexpr double f1 = __builtin_fmin(15.24, 1.3);
+ constexpr double f2 = __builtin_fmin(-0.0, +0.0);
+ constexpr double f3 = __builtin_fmin(+0.0, -0.0);
+ constexpr float f4 = __builtin_fminf(NAN, NAN);
+ constexpr float f5 = __builtin_fminf(NAN, -1);
+ constexpr float f6 = __builtin_fminf(-INFINITY, 0);
+ constexpr float f7 = __builtin_fminf(INFINITY, 0);
+
+ // frexp
+ constexpr double f8 = __builtin_frexp(123.45, &i);
+ constexpr double f9 = __builtin_frexp(0.0, &i);
+ constexpr double f10 = __builtin_frexp(-0.0, &i);
+ constexpr double f11 = __builtin_frexpf(NAN, &i);
+ constexpr double f12 = __builtin_frexpf(-NAN, &i);
+ constexpr double f13 = __builtin_frexpf(INFINITY, &i);
+ constexpr double f14 = __builtin_frexpf(INFINITY, &i);
+
+ return 0;
+}
+
+// CHECK: store double 1.300000e+00, ptr {{.*}}
+// CHECK: store double -0.000000e+00, ptr {{.*}}
+// CHECK: store double -0.000000e+00, ptr {{.*}}
+// WIN: store float 0xFFF8000000000000, ptr {{.*}}
+// LNX: store float 0x7FF8000000000000, ptr {{.*}}
+// CHECK: store float -1.000000e+00, ptr {{.*}}
+// CHECK: store float 0xFFF0000000000000, ptr {{.*}}
+
+// CHECK: store double 0x3FEEDCCCCCCCCCCD, ptr {{.*}}
+// CHECK: store double 0.000000e+00, ptr {{.*}}
+// CHECK: store double -0.000000e+00, ptr {{.*}}
+// CHECK: store double 0xFFF8000000000000, ptr {{.*}}
+// WIN: store double 0x7FF8000000000000, ptr {{.*}}
+// LNX: store double 0xFFF8000000000000, ptr {{.*}}
+// CHECK: store double 0x7FF0000000000000, ptr {{.*}}
+// CHECK: store double 0x7FF0000000000000, ptr {{.*}}
diff --git a/clang/test/SemaCXX/constexpr-math.cpp b/clang/test/SemaCXX/constexpr-math.cpp
new file mode 100644
index 00000000000000..f51ea1bcaf7bad
--- /dev/null
+++ b/clang/test/SemaCXX/constexpr-math.cpp
@@ -0,0 +1,57 @@
+// RUN: %clang_cc1 -DWIN -verify -std=c++23 -fsyntax-only %s
+// RUN: %clang_cc1 -verify -std=c++23 -fsyntax-only %s
+
+// expected-no-diagnostics
+
+
+#ifdef WIN
+#define INFINITY ((float)(1e+300 * 1e+300))
+#define NAN (-(float)(INFINITY * 0.0F))
+#else
+#define NAN (__builtin_nanf(""))
+#define INFINITY (__builtin_inff())
+#endif
+
+extern "C" void abort() noexcept;
+extern "C" int write(int, const void*, unsigned long);
+
+#define assert(condition) \
+ do { \
+ if (!(condition)) { \
+ write(2, "Assertion failed: ", 18); \
+ write(2, #condition, sizeof(#condition) - 1); \
+ write(2, "\n", 1); \
+ abort(); \
+ } \
+ } while (false)
+
+int main() {
+ int i;
+
+ // fmin
+ static_assert(__builtin_fmin(15.24, 1.3) == 1.3, "");
+ static_assert(__builtin_fmin(-0.0, +0.0) == -0.0, "");
+ static_assert(__builtin_fmin(+0.0, -0.0) == -0.0, "");
+ assert(__builtin_isnan(__builtin_fminf(NAN,NAN)));
+ assert(__builtin_isnan(__builtin_fminf(NAN, -1)));
+ assert(__builtin_isnan(__builtin_fminf(-INFINITY, 0)));
+ assert(__builtin_iszero(__builtin_fminf(+INFINITY, 0)));
+
+ // frexp
+ static_assert(__builtin_frexp(123.45, &i) == 0.96445312500000002);
+ static_assert(!__builtin_isnan(__builtin_frexp(123.45, &i)), "");
+ assert(i==0);
+ static_assert(__builtin_iszero(__builtin_frexp(0.0, &i)), "");
+ assert(i==0);
+ static_assert(__builtin_iszero(__builtin_frexp(-0.0, &i)), "");
+ assert(i==0);
+ assert(__builtin_isnan(__builtin_frexp(NAN, &i)));
+ assert(i==0);
+ assert(__builtin_isnan(__builtin_frexp(-NAN, &i)));
+ assert(i==0);
+ assert(!__builtin_isfinite(__builtin_frexp(INFINITY, &i)));
+ assert(i==0);
+ assert(!__builtin_isfinite(__builtin_frexp(-INFINITY, &i)));
+ assert(i==0);
+ return 0;
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other side notes:
fmin and frexp can signal exceptions if the input is an sNaN, which causes [library.c]p3 to kick in. (That's the only time these operations can signal an exception.)
case Builtin::BIfmin: | ||
case Builtin::BIfminf: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't make these evaluate in constexpr
. Not only does this extend C (not a stated intent of the patch), it will cause accidental dependencies that break when -fno-builtin
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have to be made constexpr
, at least in C++23. C++ stdlibs can't override them to make them call the __builtin_
versions, so Clang has to handle that.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll be missing at least "pedantic" diagnostics if we don't restrict the constexpr
-ness to C++23 and up. So we do need that restriction.
The interaction with -fno-builtin[-*]
may need more discussion. At least the documentation for that option would need to be updated if the C++23 constexpr
math would break with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the Clang C/C++ Language Workgroup call on 2024-05-15, it was agreed that -fno-builtin[-*]
should remain capable of disabling built-in treatment even when that treatment would be required for conformance w.r.t. constexpr
behaviour of functions shared with C.
For me, the first expectation for this PR (on that subject) is an update to the documentation of -fno-builtin[-*]
.
@AaronBallman @ldionne, I am no sure how we want to handle this w.r.t. documenting the C++23 implementation status. Does it go in both the Clang documentation (for the functions shared with C) and the libc++ documentation (for the overloads added by C++)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anything should happen to the libc++ documentation in this patch. We can update the implementation to behave the same across the different overloads in a follow-up and update our documentation then.
This comment was marked as outdated.
This comment was marked as outdated.
as requested by reviewer.
@hubert-reinterpretcast Thanks for the reviews/suggestions. Let me know what you think. |
@hubert-reinterpretcast Would really appreciate a review of this please. Thanks. |
clang/lib/AST/ExprConstant.cpp
Outdated
llvm::APFloat minusOne(x.getSemantics(), "-1.0"); | ||
llvm::APFloat minusHalf(x.getSemantics(), "-0.5"); | ||
llvm::APFloat half(x.getSemantics(), "0.5"); | ||
llvm::APFloat one(x.getSemantics(), "1.0"); | ||
|
||
return ((x.compare(minusOne) == llvm::APFloat::cmpGreaterThan && | ||
x.compare(minusHalf) != llvm::APFloat::cmpGreaterThan) || | ||
(x.compare(half) != llvm::APFloat::cmpLessThan && | ||
x.compare(one) == llvm::APFloat::cmpLessThan)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this by checking that the absolute value is in [0.5, 1)
@@ -253,8 +253,10 @@ def FmodF16F128 : F16F128MathTemplate, Builtin { | |||
|
|||
def FrexpF16F128 : F16F128MathTemplate, Builtin { | |||
let Spellings = ["__builtin_frexp"]; | |||
let Attributes = [FunctionWithBuiltinPrefix, NoThrow]; | |||
let Attributes = [FunctionWithBuiltinPrefix, NoThrow, Const, ConstexprSince]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frexp
family of functions cannot be marked const
because they modify the object pointed to by the pointer parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
constexpr float frexp9 = __builtin_frexpf16(3.5, &i); | ||
constexpr long double frexp10 = __builtin_frexpf128(234.78, &i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constexpr float frexp9 = __builtin_frexpf16(3.5, &i); | |
constexpr long double frexp10 = __builtin_frexpf128(234.78, &i); | |
constexpr __fp16 frexp9 = __builtin_frexpf16(3.5, &i); | |
constexpr __float128 frexp10 = __builtin_frexpf128(234.78, &i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
constexpr long double max8 = __builtin_fmaxl(123.456L, 789.012L); | ||
|
||
// frexp | ||
constexpr double frexp1 = __builtin_frexp(123.45, &i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zahiraam, to be explicit, these tests (as-is) will fail once the comment re: StoreExponent()
is addressed.
This will probably work:
constexpr double frexp1 = __builtin_frexp(123.45, &i); | |
constexpr double frexp1 = __builtin_frexp(123.45, (int [1]){}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// cplusplus20andless-error@-1 {{non-type template argument is not a constant expression}} | ||
static_assert(is_same_val<CALL_BUILTIN(frexpl, 123.45L, &i), 123.45L/128>); | ||
// cplusplus20andless-error@-1 {{non-type template argument is not a constant expression}} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the __fp16
and __float128
cases too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Ping? |
The changes to add the "constexpr since" property are missing changes to the tablegen utility. Figuring out how it would work is more involved than I have the time for right now. Also, if at all possible, I think we only want the extra field for "library builtins" and not all builtins. Alternatively, we don't need tablegen changes if we leave the encoding of which language standard a builtin is constexpr since to a helper function. |
Yes, I have tried limiting it to
Will look into this. Thanks. |
I believe that FrexpF16F128 only has the |
@hubert-reinterpretcast I have implemented what you proposed. Removed the |
const auto *FDecl = E->getDirectCallee(); | ||
clang::LangStandard::Kind ConstExprSinceVersion = | ||
FDecl->getConstexprBuiltinSinceVersion(); | ||
if (ConstExprSinceVersion > Info.Ctx.getLangOpts().LangStd) | ||
return false; | ||
LValue Pointer; | ||
if (!EvaluateFloat(E->getArg(0), Result, Info) || | ||
!EvaluatePointer(E->getArg(1), Pointer, Info)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the builtins should be unconditionally constexpr in Clang.
And we should certainly not store additional info for that in FunctionDecl
, which has a huge cost in terms of AST size.
Instead, the builtin should become constexpr, and libc++ can decide not to make the wrapper constexpr in older language modes, or to have some availability warning in older language modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cor3ntin, you mean the __builtin_
-prefixed form, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hubert-reinterpretcast I think if we wanted to do something about the non-prefixed builtin, I would prefer
- that we table-gen the information
- that the version is encoded as an attribute (it might just be an implementation detail attribute) - so as to not bloat the AST
- that we make it a diagnostic that users can turn into an error or not (and then we can discuss what the default should be)
Add support for
fmin
,fmax
, and (__builtin_
)frexp
.