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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:3846
+      New = new (*this, TypeAlignment) DependentSizedMatrixType(
+          *this, ElementTy, QualType(Canon, 0), RowExpr, ColumnExpr, AttrLoc);
+  } else {
----------------
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.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3349
+void CXXNameMangler::mangleType(const MatrixType *T) {
+  Out << "Dm" << T->getNumRows() << "_" << T->getNumColumns() << '_';
+  mangleType(T->getElementType());
----------------
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


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2109
+        if (!RowExprTemplateParam || !ColumnExprTemplateParam)
+          return Sema::TDK_Success;
+
----------------
rjmccall wrote:
> 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. :)
I wanted to make sure that's the right direction before opening too much time on refactoring :) The fact that we have to use different accessors makes things a bit tricky I think, but I've added a lambda (which takes the accessors as lambdas as well.



================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2112
+          llvm::APSInt ArgRows(S.Context.getTypeSize(S.Context.IntTy),
+                               ConstantMatrixArg->getNumRows());
+          Result = DeduceNonTypeTemplateArgument(
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281