-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Clang] Allow __builtin_fma/fmaf/fmal to be used in a constant expression #158048
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
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 still need to write tests. Trying to figure out the tablegen thing mentioned inline.
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-clang Author: Chaitanya Koparkar (ckoparkar) ChangesFixes #154747. Full diff: https://github.com/llvm/llvm-project/pull/158048.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 27639f06529cb..956e76085a08f 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -199,7 +199,7 @@ def FloorF16F128 : Builtin, F16F128MathTemplate {
def FmaF16F128 : Builtin, F16F128MathTemplate {
let Spellings = ["__builtin_fma"];
- let Attributes = [FunctionWithBuiltinPrefix, NoThrow, ConstIgnoringErrnoAndExceptions];
+ let Attributes = [FunctionWithBuiltinPrefix, NoThrow, ConstIgnoringErrnoAndExceptions, Constexpr];
let Prototype = "T(T, T, T)";
}
@@ -3885,7 +3885,7 @@ def Floor : FPMathTemplate, LibBuiltin<"math.h"> {
def Fma : FPMathTemplate, LibBuiltin<"math.h"> {
let Spellings = ["fma"];
- let Attributes = [NoThrow, ConstIgnoringErrnoAndExceptions];
+ let Attributes = [NoThrow, ConstIgnoringErrnoAndExceptions, Constexpr];
let Prototype = "T(T, T, T)";
let AddBuiltinPrefixedAlias = 1;
}
diff --git a/clang/lib/AST/ByteCode/InterpBuiltin.cpp b/clang/lib/AST/ByteCode/InterpBuiltin.cpp
index a0dcdace854b9..55baa4891bb32 100644
--- a/clang/lib/AST/ByteCode/InterpBuiltin.cpp
+++ b/clang/lib/AST/ByteCode/InterpBuiltin.cpp
@@ -3411,6 +3411,14 @@ bool InterpretBuiltin(InterpState &S, CodePtr OpPC, const CallExpr *Call,
case clang::X86::BI__builtin_ia32_pmuludq512:
return interp__builtin_ia32_pmul(S, OpPC, Call, BuiltinID);
+ case Builtin::BIfma:
+ case Builtin::BIfmal:
+ case Builtin::BIfmaf:
+ case Builtin::BI__builtin_fma:
+ case Builtin::BI__builtin_fmal:
+ case Builtin::BI__builtin_fmaf:
+ case Builtin::BI__builtin_fmaf16:
+ case Builtin::BI__builtin_fmaf128:
case Builtin::BI__builtin_elementwise_fma:
return interp__builtin_elementwise_triop_fp(
S, OpPC, Call,
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index ca930737474df..dbff682714342 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -16346,11 +16346,15 @@ bool FloatExprEvaluator::VisitCallExpr(const CallExpr *E) {
return true;
}
+ case Builtin::BIfma:
+ case Builtin::BIfmal:
+ case Builtin::BIfmaf:
+ case Builtin::BI__builtin_fma:
+ case Builtin::BI__builtin_fmal:
+ case Builtin::BI__builtin_fmaf:
+ case Builtin::BI__builtin_fmaf16:
+ case Builtin::BI__builtin_fmaf128:
case Builtin::BI__builtin_elementwise_fma: {
- if (!E->getArg(0)->isPRValue() || !E->getArg(1)->isPRValue() ||
- !E->getArg(2)->isPRValue()) {
- return false;
- }
APFloat SourceY(0.), SourceZ(0.);
if (!EvaluateFloat(E->getArg(0), Result, Info) ||
!EvaluateFloat(E->getArg(1), SourceY, Info) ||
diff --git a/clang/test/AST/ByteCode/builtin-functions.cpp b/clang/test/AST/ByteCode/builtin-functions.cpp
index f47bc49d9a1a8..961495bb2ad0a 100644
--- a/clang/test/AST/ByteCode/builtin-functions.cpp
+++ b/clang/test/AST/ByteCode/builtin-functions.cpp
@@ -1742,6 +1742,19 @@ namespace Invalid {
// both-note {{in call to}}
}
+namespace Fma {
+ static_assert(__builtin_fma(2.0, 3.0, 4.0) == 10.0);
+ static_assert(__builtin_fma(200.0, 300.0, 400.0) == 60400.0);
+ static_assert(__builtin_fmal(2.0, 3.0, 4.0) == 10.0);
+ static_assert(__builtin_fmal(200.0, 300.0, 400.0) == 60400.0);
+ static_assert(__builtin_fmaf(2.0, 3.0, 4.0) == 10.0);
+ static_assert(__builtin_fmaf(200.0, 300.0, 400.0) == 60400.0);
+ static_assert(__builtin_fmaf16(2.0, 3.0, 4.0) == 10.0);
+ static_assert(__builtin_fmaf16(200.0, 300.0, 400.0) == 60416.0);
+ static_assert(__builtin_fmaf128(2.0, 3.0, 4.0) == 10.0);
+ static_assert(__builtin_fmaf128(200.0, 300.0, 400.0) == 60400.0);
+}
+
#if __cplusplus >= 202002L
namespace WithinLifetime {
constexpr int a = 10;
|
Some libcxx tests in constexpr-cxx23-clang.pass.cpp are failing:
I'll add |
@RKSimon what I should do about the corresponding libcxx functions? |
LGTM from my side. |
I've no experience with libcxx tests - my suggestion would be to CC whoever wrote/maintained that test file |
libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
Outdated
Show resolved
Hide resolved
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 libc++ part LGTM.
Any advice on the libcxx buildbot CI failures? |
The regex/locale failures on apple platforms are unrelated - most likely due to an OS update (if we don't fix it soon we'll XFAIL the tests). The android failures should be fixed in trunk I think, but are also definitely unrelated. You can consider the CI green if no other failures in any future runs. If you're not sure feel free to ping me. |
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.
Consider:
extern "C" float fmaf(float, float, float);
#pragma STDC FENV_ACCESS ON
#pragma STDC FENV_ROUND FE_UPWARD
float f() {
return fmaf(__FLT_EPSILON__, .5f, 1.f);
}
It seems the result got folded despite potential changes to the rounding mode at run time?
$ clang++ ceFmaDynRounding.cc -emit-llvm -S -o - -Xclang -disable-llvm-passes | grep ret
ceFmaDynRounding.cc:3:34: warning: pragma STDC FENV_ROUND is not supported [-Wunknown-pragmas]
3 | #pragma STDC FENV_ROUND FE_UPWARD
| ^
ret float 0x3FF0000020000000
1 warning generated.
Return: 0x00:0 Wed Sep 17 21:24:27 2025 EDT
Also, the FENV_ROUND
pragma is working despite the warning. Tests should be added.
The constexpr evaluator uses getActiveRoundingMode to get the current rounding mode; it is used to evaluate fma, along with binary ops like *, +, -, etc. Here are the tests for fma with various rounding modes: clang/test/CodeGen/rounding-math.cpp.
Is your suggestion to add tests with different rounding modes? |
See the comment in the code for If the rounding mode is dynamic (and Indeed, the only clear situation where folding for inexact results of calls to As for manifestly constant-evaluated contexts, we do expect rounding to occur; however, whether the default rounding mode or the "constant rounding mode" should be used is a matter of design that should be backed by documentation. The documentation actively suggests that the default rounding mode is used: https://releases.llvm.org/21.1.0/tools/clang/docs/UsersManual.html#cmdoption-f-no-rounding-math. @AaronBallman, do you know if the design decision has been made and whether we have documented it? Relevant prior discussion here: https://discourse.llvm.org/t/rfc-calling-functions-if-pragma-fenv-round-is-present/79372. |
Yes, but I think we need a conclusion on the design question before knowing what such tests would look like. |
Aaron is away this month. Maybe @jcranmer-intel @rnk @erichkeane can help |
I don't have a good feel for that, there doesn't seem to be an obvious answer to me. Should we have an RFC? @andykaylor is on vacation this week, but is also very familiar with these pragmas. |
I agree, we need a rationale here. I hope some other more knowledgeable folks will weigh in soon and we can decide if we need an RFC or not. |
The topic is a little difficult to broach to the C/C++ committees because it's an interaction of aspects of each language that the other committee doesn't want to talk about for the most part, so you end up having to interpolate general design guidelines to figure out what to do.
The basic rule in C is that all floating-point operations "as if" at execution time unless they can't be done at execution time. Interpolating that to C++ semantics, then all floating-point operations are "as if" at execution time unless manifestly constant-evaluated. Frontend-level constant folding of not manifestly constant-evaluated expressions can happen only if happening at translation time is not observable, meaning:
Because of the potential for funky control bits, I lean towards the stance of prohibiting non-mandatory constant evaluation of floating-point expressions when
The C standard doesn't discuss the behavior of
That documentation appears to have been written in 2019 and not updated since then, which is before To my mind, the most defensible behavior for |
Perhaps that is how you read the intent; however, the wording is clear that standard library calls are only subject to Thank you for organizing an analysis by the way. I agree that this is an awkward topic for getting WG 14 and WG 21 clarifications on. |
|
Okay, but for the question of whether the "constant rounding mode" or "dynamic round mode" is used for this folding outside of a manifestly constant-evaluated context, I think it should actually be the "dynamic rounding mode" (i.e., default rounding with I think the |
Yes, but the PR clearly affected non- |
That is a very interesting point that I did not think about. Do we need a document to capture the information appearing in this review or do we think we'll end up with enough code-comment breadcrumbs? |
This sounds reasonable (if it is still your position after considering the macro-replacement situation). To summarize (assuming we use the same behaviour for both the prefixed and non-prefixed cases):
|
@jcranmer-intel, can you confirm whether you agree with the proposed direction in my summary? |
Fixes #154747.