Skip to content
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

[clang] Mark __builtin_convertvector and __builtin_shufflevector as constexpr. #112129

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

c8ef
Copy link
Contributor

@c8ef c8ef commented Oct 13, 2024

Closes #107985.

LanguageExtensions.rst states that __builtin_shufflevector and __builtin_convertvector can be evaluated as constants, but this is not reflected in Butiltins.td. This patch aligns these two.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 13, 2024
@c8ef c8ef requested review from RKSimon and philnik777 October 13, 2024 11:18
@llvmbot
Copy link
Member

llvmbot commented Oct 13, 2024

@llvm/pr-subscribers-clang

Author: None (c8ef)

Changes

Closes #107985.


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

1 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+2-2)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 7068473a0e12ac..362de6cfa9d0e1 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -1186,13 +1186,13 @@ def AllowRuntimeCheck : Builtin {
 
 def ShuffleVector : Builtin {
   let Spellings = ["__builtin_shufflevector"];
-  let Attributes = [NoThrow, Const, CustomTypeChecking];
+  let Attributes = [NoThrow, Const, CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def ConvertVector : Builtin {
   let Spellings = ["__builtin_convertvector"];
-  let Attributes = [NoThrow, Const, CustomTypeChecking];
+  let Attributes = [NoThrow, Const, CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 

@RKSimon
Copy link
Collaborator

RKSimon commented Oct 13, 2024

How are we already able to use these in constexpr if they are missing the attribute?

@c8ef
Copy link
Contributor Author

c8ef commented Oct 14, 2024

I believe the difference lies in the AST construction. Most built-in functions generate a CallExpr AST node, which requires the constant evaluator to check if the function is built-in. However, in this case, Clang emits a ConvertVector node directly. Therefore, even if we don't mark it as constexpr, we can still evaluate it.

@c8ef
Copy link
Contributor Author

c8ef commented Oct 14, 2024

And I think we need to align these two to make the feature test macro work.

@RKSimon
Copy link
Collaborator

RKSimon commented Oct 14, 2024

Anyway you can test that?

@c8ef
Copy link
Contributor Author

c8ef commented Oct 14, 2024

You can directly view it from the Clang AST on Godbolt. For this patch, I don't believe additional tests are necessary.

@philnik777
Copy link
Contributor

You can directly view it from the Clang AST on Godbolt. For this patch, I don't believe additional tests are necessary.

Any bugfix should contain a regression test. Here you could simply check that __has_constexpr_builtin is true for these builtins, since that is the most obvious behaviour change.

@c8ef c8ef merged commit ab6ec7a into llvm:main Oct 14, 2024
8 checks passed
@c8ef c8ef deleted the vector-const branch October 14, 2024 16:09
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
…as `constexpr`. (llvm#112129)

Closes llvm#107985.

LanguageExtensions.rst states that `__builtin_shufflevector` and
`__builtin_convertvector` can be evaluated as constants, but this is not
reflected in Butiltins.td. This patch aligns these two.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
…as `constexpr`. (llvm#112129)

Closes llvm#107985.

LanguageExtensions.rst states that `__builtin_shufflevector` and
`__builtin_convertvector` can be evaluated as constants, but this is not
reflected in Butiltins.td. This patch aligns these two.
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
…as `constexpr`. (llvm#112129)

Closes llvm#107985.

LanguageExtensions.rst states that `__builtin_shufflevector` and
`__builtin_convertvector` can be evaluated as constants, but this is not
reflected in Butiltins.td. This patch aligns these two.
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.

[Clang] constexpr support for __builtin_convertvector and __builtin_shufflevector
4 participants