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][Builtins] Parse clang extended vectors types. #83584

Conversation

fpetrogalli
Copy link
Member

@fpetrogalli fpetrogalli commented Mar 1, 2024

Clang extended vector types are mangled as follows:

'_ExtVector<' <lanes> ',' <scalar type> '>'

This is used to defetmine the builtins signature for builtins that
use parameters defined as

typedef <scalar type> ext_vector_type_<lanes>_<scalar type> __attribute__((ext_vector_type(<lanes>)))

or

template <unsigned N, class T>
using _ExtVector __attribute__((ext_vector_type(N))) = T;

For example:

typedef double ext_vector_type_4_double __attribute__((ext_vector_type(4)))

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Mar 1, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 1, 2024

@llvm/pr-subscribers-clang

Author: Francesco Petrogalli (fpetrogalli)

Changes

Clang extended vector types are mangled as follows:

ext_vector_type_<lanes>_<scalar type>

This is used to defetmine the builtins signature for builtins that use parmeters defined as

typedef &lt;scalar type&gt; ext_vector_type_&lt;lanes&gt;_&lt;scalar type&gt; __attribute__((ext_vector_type(&lt;lanes&gt;)))

For example:

typedef double ext_vector_type_4_double __attribute__((ext_vector_type(4)))

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

2 Files Affected:

  • (added) clang/test/TableGen/target-builtins-prototype-parser.td (+20)
  • (modified) clang/utils/TableGen/ClangBuiltinsEmitter.cpp (+10)
diff --git a/clang/test/TableGen/target-builtins-prototype-parser.td b/clang/test/TableGen/target-builtins-prototype-parser.td
new file mode 100644
index 00000000000000..681a607da7e115
--- /dev/null
+++ b/clang/test/TableGen/target-builtins-prototype-parser.td
@@ -0,0 +1,20 @@
+// RUN: clang-tblgen -I %p/../../../clang/include/ %s --gen-clang-builtins | FileCheck %s
+// RUN: not clang-tblgen -I %p/../../../clang/include/ %s --gen-clang-builtins -DERROR_EXPECTED_LANES 2>&1 | FileCheck %s --check-prefix=ERROR_EXPECTED_LANES
+
+include "clang/Basic/BuiltinsBase.td"
+
+def : Builtin {
+  let Prototype = "ext_vector_type_8_int(double, ext_vector_type_4_bool)";
+  let Spellings = ["__builtin_test_use_clang_extended_vectors"];
+}
+
+// CHECK: BUILTIN(__builtin_test_use_clang_extended_vectors, "E8idE4b", "")
+
+#ifdef ERROR_EXPECTED_LANES
+def : Builtin {
+// ERROR_EXPECTED_LANES: :[[# @LINE + 1]]:7: error: Expected number of lanes
+  let Prototype = "ext_vector_type__int(double, ext_vector_type_4_bool)";
+  let Spellings = ["__builtin_test_use_clang_extended_vectors"];
+}
+#endif
+
diff --git a/clang/utils/TableGen/ClangBuiltinsEmitter.cpp b/clang/utils/TableGen/ClangBuiltinsEmitter.cpp
index 48f55b8af97e4e..774f703390a05e 100644
--- a/clang/utils/TableGen/ClangBuiltinsEmitter.cpp
+++ b/clang/utils/TableGen/ClangBuiltinsEmitter.cpp
@@ -85,6 +85,16 @@ class PrototypeParser {
       if (Substitution.empty())
         PrintFatalError(Loc, "Not a template");
       ParseType(Substitution);
+    } else if (T.consume_front("ext_vector_type_")) {
+      // Clang extended vector types are mangled as follows:
+      //
+      //    ext_vector_type_<lanes>_<scalar type>
+      unsigned long long Lanes;
+      if (llvm::consumeUnsignedInteger(T, 10, Lanes))
+        PrintFatalError(Loc, "Expected number of lanes ");
+      Type += "E" + std::to_string(Lanes);
+      T.consume_front("_");
+      ParseType(T);
     } else {
       auto ReturnTypeVal = StringSwitch<std::string>(T)
                                .Case("__builtin_va_list_ref", "A")

@fpetrogalli
Copy link
Member Author

@philnik777 - thank you for the patch at #68324

I am extending the parser to be able to recognise clang extended vectors.

Thanks!

Francesco

@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Mar 1, 2024
@fpetrogalli fpetrogalli force-pushed the handle-clang-extended-vectors-in-tabelgen-builtin-backend branch from 5a2c8ba to e7ac017 Compare March 1, 2024 15:47
@philnik777
Copy link
Contributor

@philnik777 - thank you for the patch at #68324

You're welcome!

FWIW I'd find a syntax like _ExtVector<bool, 4> better. The underscore and upper case to make it clear that it's non-standard and the angle bracket syntax since it's kind-of a template. This unfortunately doesn't map really nicely to anything native otherwise. Maybe think of it like

template <class T, unsigned N>
using _ExtVector __attribute__((ext_vector_type(N))) = T;

Then you'd actually use it as _ExtVector<bool, 4> in source code.
I don't think that would be significantly harder to parse.

Copy link

github-actions bot commented Mar 1, 2024

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

@fpetrogalli
Copy link
Member Author

@philnik777 - thank you for the patch at #68324

You're welcome!

FWIW I'd find a syntax like _ExtVector<bool, 4> better. The underscore and upper case to make it clear that it's non-standard and the angle bracket syntax since it's kind-of a template. This unfortunately doesn't map really nicely to anything native otherwise. Maybe think of it like

template <class T, unsigned N>
using _ExtVector __attribute__((ext_vector_type(N))) = T;

Then you'd actually use it as _ExtVector<bool, 4> in source code. I don't think that would be significantly harder to parse.

I am almost there. Before going a bit wider with tests I wanted to let you know that, to keep the parsing operation simple, I have opted for _ExtVector<N:T> [1]. The reason being that the token ',' is already used to split the parameters of the prototype.

Shall we rework the parser to make it compatible with template-style parameters? I like the idea a lot, I am just not sure it is worth pursuing this extra complication.

Francesco

[1] It will become _ExtVector<T:N> if I get your blessing for the split with ':' instead of ','.

@fpetrogalli
Copy link
Member Author

Other C++ compatible option: provide a family of templates, one for each number of lanes, templated on the type:

_ExtVector_N<T>

So that we need up having:

_ExtVector_2<double>
_ExtVector_6<unsigned int>
...

This would be pretty easy to handle, without introducing extra complications in the parser.

@philnik777
Copy link
Contributor

Hm, yeah. I don't think it's worth complicating the parser for a tiny bit of syntax sugar. I like your idea with a : quite a bit. _ExtVector(bool:4) would also be an option. I don't have a strong preference either way. I'd like to keep it separate from the name though to make it obvious that it's a parameter (at least it feels to me like that).

@francisvm
Copy link
Collaborator

Maybe we can agree to not allow nested <...>, and it should simplify the parsing a little?

Clang extended vector types are mangled as follows:

   '_ExtVector<' <lanes> ',' <scalar type> '>'

This is used to defetmine the builtins signature for builtins that
use parameters defined as

    typedef <scalar type> ext_vector_type_<lanes>_<scalar type> __attribute__((ext_vector_type(<lanes>)))

For example:

    typedef double ext_vector_type_4_double __attribute__((ext_vector_type(4)))
@fpetrogalli fpetrogalli force-pushed the handle-clang-extended-vectors-in-tabelgen-builtin-backend branch from ab12de3 to 53d9fe7 Compare March 4, 2024 12:43
@fpetrogalli
Copy link
Member Author

fpetrogalli commented Mar 4, 2024

@francisvm / @philnik777 - I have opted for:

'_ExtVector<' <lanes> ',' <scalar type> '>'

The reason for specifying <lanes> first is due to the fact that it makes it simpler to build the final string, because the ParseType function is recursive.

Copy link
Collaborator

@francisvm francisvm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from some extra test coverage and maybe a comment to be added.

clang/utils/TableGen/ClangBuiltinsEmitter.cpp Outdated Show resolved Hide resolved
@fpetrogalli fpetrogalli merged commit 9b672de into llvm:main Mar 5, 2024
4 checks passed
@tuliom
Copy link
Contributor

tuliom commented Mar 6, 2024

@fpetrogalli I found a small issue with this PR. Could you take a look at PR #84186 and review it, please?

tuliom added a commit that referenced this pull request Mar 6, 2024
…e-parser.td (#84186)

Use a path that works for both standalone as well as full repository
builds.

Fixes: 9b672de ("[clang][Builtins] Parse clang extended vectors types. (#83584)")
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.

6 participants