rjmccall added a comment.
The test cases for function template partial specialization would look
something like this:
template <class T, size_t R, size_t C>
using matrix = T __attribute__((matrix_type(R, C)));
template <int N> struct selector {};
template <class T, size_t R, size_t C>
selector<0> use_matrix(matrix<T, R, C> m) {}
template <class T, size_t R>
selector<1> use_matrix(matrix<T, R, 10> m) {}
template <class T>
selector<2> use_matrix(matrix<T, 10, 10> m) {}
void test() {
selector<2> x = use_matrix(matrix<int, 10, 10>());
selector<1> y = use_matrix(matrix<int, 12, 10>());
selector<0> z = use_matrix(matrix<int, 12, 12>());
}
But you should include some weirder kinds of template, expressions that aren't
deducible, and so on.
================
Comment at: clang/lib/AST/ASTContext.cpp:3840
+ RowExpr, ColumnExpr, AttrLoc);
+ } else {
+ QualType CanonicalMatrixElementType = getCanonicalType(MatrixElementType);
----------------
fhahn wrote:
> rjmccall wrote:
> > Is this logic copied from the dependent vector code? It's actually wrong
> > in a sense — it'll end up creating a non-canonical matrix type wrapping
> > Canon even if all the components are exactly the same. Probably the only
> > impact is that we waste a little memory, although it's also possible that
> > type-printing in diagnostics will be a little weird. It'd be better to
> > recognize that the components match Canon exactly and avoid creating a
> > redundant node.
> >
> > Please cache the result of calling `getCanonicalType(MatrixElementType)`
> > above.
> > Is this logic copied from the dependent vector code? It's actually wrong in
> > a sense — it'll end up creating a non-canonical matrix type wrapping Canon
> > even if all the components are exactly the same. Probably the only impact
> > is that we waste a little memory, although it's also possible that
> > type-printing in diagnostics will be a little weird. It'd be better to
> > recognize that the components match Canon exactly and avoid creating a
> > redundant node.
>
> Yes I think it is similar to what the code for dependent vectors does. I've
> updated it to use the Canonical type, it the components match exactly. If
> that properly addresses your comment I can submit a patch to do the same for
> the vector case.
This looks right except that you don't want to add it again to the `Types`
list. You can just early-exit if you don't have to build anything new.
================
Comment at: clang/lib/AST/ASTContext.cpp:3846
+ New = new (*this, TypeAlignment) DependentSizedMatrixType(
+ *this, ElementTy, QualType(Canon, 0), RowExpr, ColumnExpr, AttrLoc);
+ } else {
----------------
Some of this redundancy is avoidable. I think you can just structure this as:
- Look for an existing canonical matrix type.
- If you find one, and it matches exactly, return it.
- If you don't have a canonical matrix type, build it and add it to both
`DependentSizedMatrixTypes` and `Types`.
- You now have a canonical matrix type; if it doesn't match exactly (you can
remember a flag for this), build a non-canonical matrix type (and add it to
`Types`).
- Return the non-canonical matrix type if you built one, or otherwise the
canonical matrix type.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3349
+void CXXNameMangler::mangleType(const MatrixType *T) {
+ Out << "Dm" << T->getNumRows() << "_" << T->getNumColumns() << '_';
+ mangleType(T->getElementType());
----------------
fhahn wrote:
> rjmccall wrote:
> > fhahn wrote:
> > > rjmccall wrote:
> > > > fhahn wrote:
> > > > > rjmccall wrote:
> > > > > > You need to ask the Itanium C++ ABI for this mangling, since it's
> > > > > > not using a vendor-extension prefix. I know that wasn't done for
> > > > > > vector types, but we're trying to do the right thing.
> > > > > That basically means reaching out to
> > > > > https://github.com/itanium-cxx-abi/cxx-abi/, right?
> > > > >
> > > > > Do you think it would be feasible to initially go with a
> > > > > vendor-extensions mangling (like
> > > > > `u<Lenght>Matrix<NumRows>x<NumColumns>x<ElementType>`)? I've updated
> > > > > the mangling to this.
> > > > Yeah, you can open an issue there.
> > > >
> > > > That approach doesn't quite work because mangling the element type can
> > > > use or introduce substitutions, but the demangler will naturally skip
> > > > the whole thing. I think there are two possible approaches here: we
> > > > could either treat the entire matrix type as a vendor-extended
> > > > qualifier on the element type (i.e. `U11matrix_typeILi3ELi4EEi`), or we
> > > > could extend the vendor-extended type mangling to allow template-args
> > > > (i.e. `u11matrix_typeIiLi3ELi4EE`). The latter is probably better and
> > > > should fit cleanly into the mangling grammar.
> > > Thanks for those suggestions. For now I went with the vendor-extended
> > > qualifier, because IIUC that would fit in the existing mangling scheme
> > > without any changes, while the second option would require changes to the
> > > grammar, right?
> > >
> > Yes, but it's a very modest change; please raise this with the Itanium
> > committee (which is largely just me again, but wearing a different hat).
> >
> > In the meantime, the qualifier approach is fine as a hack, but it's not
> > technically correct unless you do annoying things with the mangling of
> > actually-qualified matrix types. But the proper qualifier approach is to
> > provide the arguments as template-arguments, not one big unstructured
> > string.
> >
> > Also you should do the same thing with DependentSizedMatrixType.
> > Yes, but it's a very modest change; please raise this with the Itanium
> > committee (which is largely just me again, but wearing a different hat).
>
> Great, I will do so shortly (probably tomorrow).
>
> > Also you should do the same thing with DependentSizedMatrixType.
>
> I am not sure if it would make sense for DependentSizedMatrixType, as we
> would have to mangle both the row and column expressions and add them to the
> qualifier IIUC. I've disabled mangling for DependentSizedMatrixType for now.
The qualifier production is `U <source-name> <template-args>?`, where the
intent of the `<template-args>` is to capture any arguments you might have. So
instead of building one big string with all the sizes in it, you should build
just the constant string `matrix_type` and then add `I...E` with the arguments.
For the constant case, you can just call
`mangleIntegerLiteral(Context.getSizeType(), T->getNumRows())`; I think
`size_t` is the appropriate presumed type for these bounds. For the dependent
case, you should actually call `mangleTemplateArgument` passing the expression,
which should promote to `TemplateArgument` implicitly.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2109
+ if (!RowExprTemplateParam || !ColumnExprTemplateParam)
+ return Sema::TDK_Success;
+
----------------
fhahn wrote:
> rjmccall wrote:
> > You should just do this right. If you can find a template parameter in the
> > parameter's row/column expression, you have to try to deduce it, and
> > short-circuit if that fails. Deduction is order-agnostic, so don't worry
> > about that.
> >
> > Also, you need to handle DependentSizedMatrixTypes here in order to get
> > function template partial ordering right. Pattern the code on how
> > DependentSizedArrayTypes would handle it.
> Thanks, I've re-structured the code along the lines of the code for
> DependentSizedArrayType
Could you extract a lambda helper function that does the common logic for the
rows/columns values? It's probably cleanest to pass in a rows/cols flag
instead of trying to abstract more than that.
If you do that, you'll also fix the bug where you're using ColumnNTTP in the
rows case. :)
================
Comment at: clang/lib/Sema/SemaType.cpp:6122
+ llvm_unreachable(
+ "no address_space attribute found at the expected location!");
+}
----------------
Well, I would hope not. :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72281/new/
https://reviews.llvm.org/D72281
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
