Skip to content

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

@fpetrogalli fpetrogalli requested a review from philnik777 March 1, 2024 15:34
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Mar 1, 2024
@llvmbot
Copy link
Member

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.

@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