Skip to content

[clang][bytecode] Implement the constexpr built-in abs function. #112459

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

Closed
wants to merge 7 commits into from

Conversation

c8ef
Copy link
Contributor

@c8ef c8ef commented Oct 16, 2024

The current built-in abs function cannot be used in a constexpr environment. This patch fixes the issue.
See also: https://godbolt.org/z/s8x3Yd8ne.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 16, 2024
@c8ef c8ef requested a review from tbaederr October 16, 2024 01:24
@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-clang

Author: None (c8ef)

Changes

The current built-in abs function cannot be used in a constexpr environment. This patch fixes the issue.
See also: https://godbolt.org/z/s8x3Yd8ne.


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

3 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+1)
  • (modified) clang/lib/AST/ExprConstant.cpp (+11)
  • (modified) clang/test/AST/ByteCode/builtin-functions.cpp (+9)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index bda8a48be92bda..e1b4d5b1fdc0a5 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -2714,6 +2714,7 @@ def Abs : IntMathTemplate, LibBuiltin<"stdlib.h"> {
   let Attributes = [NoThrow, Const];
   let Prototype = "T(T)";
   let AddBuiltinPrefixedAlias = 1;
+  let OnlyBuiltinPrefixedAliasIsConstexpr = 1;
 }
 
 def Calloc : LibBuiltin<"stdlib.h"> {
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 52a7f5778ce6d2..69539a7f03feba 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -13055,6 +13055,17 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
     return Success(Val.popcount() % 2, E);
   }
 
+  case Builtin::BI__builtin_abs:
+  case Builtin::BI__builtin_labs:
+  case Builtin::BI__builtin_llabs: {
+    APSInt Val;
+    if (!EvaluateInteger(E->getArg(0), Val, Info))
+      return false;
+    if (Val.isNegative())
+      Val.negate();
+    return Success(Val, E);
+  }
+
   case Builtin::BI__builtin_popcount:
   case Builtin::BI__builtin_popcountl:
   case Builtin::BI__builtin_popcountll:
diff --git a/clang/test/AST/ByteCode/builtin-functions.cpp b/clang/test/AST/ByteCode/builtin-functions.cpp
index 450ff5671314db..46e5b0579bd575 100644
--- a/clang/test/AST/ByteCode/builtin-functions.cpp
+++ b/clang/test/AST/ByteCode/builtin-functions.cpp
@@ -265,6 +265,15 @@ namespace fpclassify {
   char classify_subnorm [__builtin_fpclassify(-1, -1, -1, +1, -1, 1.0e-38f)];
 }
 
+namespace abs {
+static_assert(__builtin_abs(14) == 14, "");
+static_assert(__builtin_labs(14L) == 14L, "");
+static_assert(__builtin_llabs(14LL) == 14LL, "");
+static_assert(__builtin_abs(-14) == 14, "");
+static_assert(__builtin_labs(-14L) == 14L, "");
+static_assert(__builtin_llabs(-14LL) == 14LL, "");
+} // namespace abs
+
 namespace fabs {
   static_assert(__builtin_fabs(-14.0) == 14.0, "");
 }

@c8ef c8ef requested a review from yronglin October 16, 2024 01:24
@tbaederr
Copy link
Contributor

This patch fixes the issue.

What issue? Is there a bug open about this?

You didn't implement it for the bytecode interpreter at all, only for the current interpreter.

@c8ef
Copy link
Contributor Author

c8ef commented Oct 16, 2024

This patch fixes the issue.

What issue? Is there a bug open about this?

You didn't implement it for the bytecode interpreter at all, only for the current interpreter.

Yes, I realized that. Apologies for the oversight. I will add the implementation to InterpBuiltin.cpp.

Copy link

github-actions bot commented Oct 16, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@c8ef
Copy link
Contributor Author

c8ef commented Oct 16, 2024

This issue arose when I was trying to address #51787, which concerns vector operations built-ins. I wanted to see how the single versions were implemented, only to discover that __builtin_abs cannot be evaluated in a constexpr context. So, I tried to fix it.

@c8ef c8ef marked this pull request as draft October 16, 2024 10:07
@c8ef c8ef closed this Oct 16, 2024
@c8ef c8ef deleted the abs branch October 16, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants