[PATCH] D72281: [Matrix] Add matrix type to Clang.

[John McCall via Phabricator via cfe-commits]

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