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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> The type of m1 matches the matrix argument in all 3 definitions of use_matrix and for some reason the return type is not used to disambiguate the definitions:

C++ does not have the ability to use return types to disambiguate function calls.

> I am not sure where things are going wrong unfortunately. The matrix argument deduction should mirror the code for types like DependentSizedArrayType. Do you have any idea what could be missing?

This is what function template partial ordering is supposed to do.  You'll see that it works if you replace the definition of `matrix` to use array types:

  template <class T, size_t R, size_t C>
  using matrix = T (*)[R][C];

In function template partial ordering, the dependent parameter types of one function template are used as "arguments" to try to deduce arguments for the template parameters of another, and if that succeeds in one direction and not in the other, the first template is considered to be more specialized.  Try isolating individual pairs and then seeing why the ordering-deduction is failing.



================
Comment at: clang/lib/AST/ASTContext.cpp:3846
+      New = new (*this, TypeAlignment) DependentSizedMatrixType(
+          *this, ElementTy, QualType(Canon, 0), RowExpr, ColumnExpr, AttrLoc);
+  } else {
----------------
fhahn wrote:
> rjmccall wrote:
> > 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.
> Thanks for the suggestion! I've updated the code accordingly.
Looks great, thanks.


================
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:
> > > > > > 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.
> Ah right, I thought we have to provide the length of the full qualifier (including the arguments), but it should be OK to just provide the size of matrix_type and then mangle the arguments directly. Updated.
> 
> I also filed an issue for the mangling: https://github.com/itanium-cxx-abi/cxx-abi/issues/100
Looks good, thanks!  Although please use a `StringRef` here instead of copying into a `std::string`.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2112
+          llvm::APSInt ArgRows(S.Context.getTypeSize(S.Context.IntTy),
+                               ConstantMatrixArg->getNumRows());
+          Result = DeduceNonTypeTemplateArgument(
----------------
fhahn wrote:
> rjmccall wrote:
> > Oh, and this needs to use `size_t`; not just the value but the type of a non-type template parameter can be deduced, so you should use an appropriate type.  Test case for this deduction is `template <auto R> void foo(matrix<int, R, 10>);` or something like that, with a check that `decltype(R)` is `size_t` (which you can do in any number of ways).
> Could we be even more permissive? If we fix it to size_t, template arguments with integer types like int or unsigned would be rejected.  Could we relax that to NTTP type, to allow different integer types that are implicitly convertible? We might need an extra check that the known number of rows/columns does not exceed the bit width of NTTP's type.
Unfortunately, template argument deduction requires the template parameter type to match the type of the argument value exactly, so you get exactly one type.  Given that language rule, the natural type to use is `size_t`, which is the same type that constant array bounds are considered to have.   If C++ wants to weaken this language rule, they should do so uniformly; it does not make sense for us to use a weaker rule just for this specific extension.


================
Comment at: clang/lib/Sema/SemaType.cpp:6122
+  llvm_unreachable(
+      "no address_space attribute found at the expected location!");
+}
----------------
fhahn wrote:
> rjmccall wrote:
> > Well, I would hope not. :)
> Also just fillMatrixTypeLoc should be sufficient, as both ConstantMatrixTypeLoc and DependentSizedMatrixTypeLoc inherit from it.
Ah, good point, since there are no source differences between the two cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72281/new/

https://reviews.llvm.org/D72281