Skip to content

Conversation

ckoparkar
Copy link
Contributor

Fixes #154747.

Copy link
Contributor Author

@ckoparkar ckoparkar left a 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.

@ckoparkar ckoparkar marked this pull request as ready for review September 13, 2025 15:27
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:bytecode Issues for the clang bytecode constexpr interpreter labels Sep 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2025

@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang

Author: Chaitanya Koparkar (ckoparkar)

Changes

Fixes #154747.


Full diff: https://github.com/llvm/llvm-project/pull/158048.diff

4 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+2-2)
  • (modified) clang/lib/AST/ByteCode/InterpBuiltin.cpp (+8)
  • (modified) clang/lib/AST/ExprConstant.cpp (+8-4)
  • (modified) clang/test/AST/ByteCode/builtin-functions.cpp (+13)
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;

@ckoparkar
Copy link
Contributor Author

Some libcxx tests in constexpr-cxx23-clang.pass.cpp are failing:

ASSERT_NOT_CONSTEXPR_CXX23(std::fma(1.0f, 1.0f, 1.0f) == 2.0f);
ASSERT_NOT_CONSTEXPR_CXX23(std::fma(1.0, 1.0, 1.0) == 2.0);
ASSERT_NOT_CONSTEXPR_CXX23(std::fma(1.0L, 1.0L, 1.0L) == 2.0L);
ASSERT_NOT_CONSTEXPR_CXX23(std::fmaf(1.0f, 1.0f, 1.0f) == 2.0f);
ASSERT_NOT_CONSTEXPR_CXX23(std::fmal(1.0L, 1.0L, 1.0L) == 2.0L); 

I'll add __has_constexpr_builtin(__builtin_signbit) to fix the tests. Should I also add _LIBCPP_CONSTEXPR_SINCE_CXX23 to the libcxx header and do other things similar to #105946?

@ckoparkar ckoparkar requested a review from a team as a code owner September 13, 2025 22:45
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 13, 2025
@ckoparkar
Copy link
Contributor Author

@RKSimon what I should do about the corresponding libcxx functions?

@RKSimon RKSimon requested review from tbaederr and RKSimon September 16, 2025 19:11
@tbaederr
Copy link
Contributor

LGTM from my side.

@RKSimon
Copy link
Collaborator

RKSimon commented Sep 17, 2025

@RKSimon what I should do about the corresponding libcxx functions?

I've no experience with libcxx tests - my suggestion would be to CC whoever wrote/maintained that test file

@cor3ntin
Copy link
Contributor

@philnik777

Copy link
Contributor

@philnik777 philnik777 left a 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.

@RKSimon
Copy link
Collaborator

RKSimon commented Sep 17, 2025

The libc++ part LGTM.

Any advice on the libcxx buildbot CI failures?

@philnik777
Copy link
Contributor

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.

Copy link
Collaborator

@hubert-reinterpretcast hubert-reinterpretcast left a 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.

@ckoparkar
Copy link
Contributor Author

It seems the result got folded despite potential changes to the rounding mode at run time?

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.

Also, the FENV_ROUND pragma is working despite the warning. Tests should be added.

Is your suggestion to add tests with different rounding modes?

@hubert-reinterpretcast
Copy link
Collaborator

hubert-reinterpretcast commented Sep 19, 2025

The constexpr evaluator uses getActiveRoundingMode to get the current rounding mode; it is used to evaluate fma, along with binary ops like *, +, -, etc.

See the comment in the code for getActiveRoundingMode.

If the rounding mode is dynamic (and FENV_ACCESS is "on"), it is only acceptable to fold cases that are not manifestly constant-evaluated if the result is exact.

Indeed, the only clear situation where folding for inexact results of calls to fma outside of a manifestly constant-evaluated context is acceptable is when FENV_ROUND is explicitly the default rounding mode (FE_TONEAREST) and FENV_ACCESS is "off" (thus making the "constant rounding mode" match the presumed "dynamic rounding mode"). FENV_ROUND is specified, by the C standard, to affect the behaviour of fma by means of macro replacement: a call that avoids the macro replacement, e.g., (fma)(x, y, z) is supposed to operate using the dynamic rounding mode.

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.

@hubert-reinterpretcast
Copy link
Collaborator

Is your suggestion to add tests with different rounding modes?

Yes, but I think we need a conclusion on the design question before knowing what such tests would look like.

@cor3ntin
Copy link
Contributor

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.

Aaron is away this month. Maybe @jcranmer-intel @rnk @erichkeane can help

@erichkeane
Copy link
Collaborator

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.

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.

@shafik
Copy link
Collaborator

shafik commented Sep 19, 2025

Is your suggestion to add tests with different rounding modes?

Yes, but I think we need a conclusion on the design question before knowing what such tests would look like.

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.

@jcranmer-intel
Copy link
Contributor

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.

If the rounding mode is dynamic (and FENV_ACCESS is "on"), it is only acceptable to fold cases that are not manifestly constant-evaluated if the result is exact.

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:

  • If FENV_ACCESS is off and FENV_ROUND is missing, we are in the fully default FP environments. FP exceptions are not visible, no funky control bits are set, and the rounding mode is default. It is safe to do the translation at compile-time.
  • If FENV_ACCESS is off and FENV_ROUND is present, then we are in the fully default FP environment except that the rounding mode isn't necessarily in the default rounding mode. We can do the translation at compile-time, even if FENV_ROUND is FE_DYNAMIC (since the translator is explicitly allowed to assume that it's in the default round per the C specification).
  • If FENV_ACCESS is on and FENV_ROUND is missing, we are in an arbitrary FP environment. FP exceptions are visible, funky control bits may be set, and the rounding mode may be in any state. It is not safe to do the translation at compile-time, since we cannot guarantee that a weird control bit (like x87 precision control) is not in play.
  • If FENV_ACCESS is on and FENV_ROUND is present, then we at least know the rounding mode. But since we still can't assume the absence of funky control bits, it's not safe to do the translation at compile-time.

Because of the potential for funky control bits, I lean towards the stance of prohibiting non-mandatory constant evaluation of floating-point expressions when FENV_ACCESS is on, even if the expression would be exact. But FENV_ACCESS being off lets us exercise the as-if rule pretty freely.

FENV_ROUND is specified, by the C standard, to affect the behaviour of fma by means of macro replacement: a call that avoids the macro replacement, e.g., (fma)(x, y, z) is supposed to operate using the dynamic rounding mode.

The C standard doesn't discuss the behavior of __builtin_fma, so we have to interpolate what its behavior should be from the more general rules of FENV_ROUND. The way I read the intent is that FENV_ROUND applies the behavior of the rounding mode to all known operations and restores the dynamic rounding mode for all unknown function calls, with standard library calls being considered to be a known operation when they are not macro-replaced.

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.

That documentation appears to have been written in 2019 and not updated since then, which is before FENV_ROUND was incorporated into C2x, long before any work was done in Clang to support FENV_ROUND. Especially as Clang's FENV_ROUND implementation is still incomplete enough that we don't claim to support it yet, I wouldn't take it as any definitive evidence of a decision having been made.

To my mind, the most defensible behavior for __builtin_fma (and future similar builtins) is this: they have the same behavior with respect to constant expression evaluation, rounding mode, and strict FP mode (i.e., FENV_ACCESS) as a humble + operator. That is, in the cases where they are manifestly constant-evaluated, they take on the rounding mode implied by FENV_ROUND.

@hubert-reinterpretcast
Copy link
Collaborator

hubert-reinterpretcast commented Sep 19, 2025

The way I read the intent is that FENV_ROUND applies the behavior of the rounding mode to all known operations and restores the dynamic rounding mode for all unknown function calls, with standard library calls being considered to be a known operation when they are not macro-replaced.

Perhaps that is how you read the intent; however, the wording is clear that standard library calls are only subject to FENV_ROUND behaviour when they are macro-replaced (by macros defined by the C library headers).

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.

@jcranmer-intel
Copy link
Contributor

Perhaps that is how you read the intent; however, the wording is clear that standard library calls are only subject to FENV_ROUND behaviour when they are macro-replaced (by macros defined by the C library headers).

__builtin_fma is not a standard library call, nor is it a function call.

@hubert-reinterpretcast
Copy link
Collaborator

  • If FENV_ACCESS is off and FENV_ROUND is present, then we are in the fully default FP environment except that the rounding mode isn't necessarily in the default rounding mode. We can do the translation at compile-time, even if FENV_ROUND is FE_DYNAMIC (since the translator is explicitly allowed to assume that it's in the default round per the C specification).

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 FENV_ROUND effectively ignored) if the call is not __builtin_-prefixed. This goes back to the responsibility of some header to modify calls that are subject to FENV_ROUND-controlled behaviour via macro replacement (e.g., to call a different built-in).

I think the __builtin_-prefixed cases should behave the same unless there is a compelling reason to do otherwise.

@hubert-reinterpretcast
Copy link
Collaborator

hubert-reinterpretcast commented Sep 19, 2025

__builtin_fma is not a standard library call, nor is it a function call.

Yes, but the PR clearly affected non-__builtin_-prefixed calls. See #158048 (review).

@hubert-reinterpretcast
Copy link
Collaborator

I lean towards the stance of prohibiting non-mandatory constant evaluation of floating-point expressions when FENV_ACCESS is on, even if the expression would be exact.

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?

@hubert-reinterpretcast
Copy link
Collaborator

To my mind, the most defensible behavior for __builtin_fma (and future similar builtins) is this: they have the same behavior with respect to constant expression evaluation, rounding mode, and strict FP mode (i.e., FENV_ACCESS) as a humble + operator. That is, in the cases where they are manifestly constant-evaluated, they take on the rounding mode implied by FENV_ROUND.

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):

  • manifestly constant-evaluated:
    • obey FENV_ROUND; FE_DYNAMIC is treated the same as default (i.e., to nearest)
      • note: FENV_ROUND affects the invocations lexically within its scope (i.e., it does not transfer from caller to callee, and it has the same challenges with default function arguments and default member initializers as FENV_ACCESS)
  • not manifestly constant-evaluated:
    • FENV_ACCESS is "on": do not evaluate in the compiler
    • FENV_ACCESS is "off": disregard FENV_ROUND; use the default rounding mode (expect cases that should obey FENV_ROUND to be macro replaced)

@hubert-reinterpretcast
Copy link
Collaborator

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?
@andykaylor, your input would be appreciated.

@tbaederr
Copy link
Contributor

tbaederr commented Oct 7, 2025

@jcranmer-intel @andykaylor ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:bytecode Issues for the clang bytecode constexpr interpreter clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang] Allow __builtin_fma/fmaf/fmal to be used in a constant expression
10 participants