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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/TypeNodes.td:73
 def ExtVectorType : TypeNode<VectorType>;
+def MatrixType : TypeNode<Type>;
 def FunctionType : TypeNode<Type, 1>;
----------------
I think some parts of your life might be easier if there were a common base class here.  You can either call the abstract superclass something like `AbstractMatrixType`, or you can call it `MatrixType` and then make this the concrete subclass `ConstantMatrixType`.

This is something we've occasionally regretted with vector types.


================
Comment at: clang/lib/AST/ASTContext.cpp:3840
+                                 RowExpr, ColumnExpr, AttrLoc);
+  } else {
+    QualType CanonicalMatrixElementType = getCanonicalType(MatrixElementType);
----------------
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.


================
Comment at: clang/lib/AST/ASTContext.cpp:3848
+      assert(!CanonCheck && "Dependent-sized matrix canonical type broken");
+      (void)CanonCheck;
+      DependentSizedMatrixTypes.InsertNode(New, InsertPos);
----------------
Please do this in an `#ifndef NDEBUG`.


================
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:
> > > > 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.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2071
+            MatrixArg->getElementType(), Info, Deduced, TDF);
+      }
+      // Matrix-Matrix deduction.
----------------
This should just fail with `TDK_NonDeducedMismatch`, like a ConstantArrayType would if the argument was a DependentSizedArrayType.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2109
+        if (!RowExprTemplateParam || !ColumnExprTemplateParam)
+          return Sema::TDK_Success;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281