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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked an inline comment as done.
fhahn 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>;
----------------
rjmccall wrote:
> 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.
I've added ConstantMatrixType and DependentSizedMatrixType, which are both subclasses of MatrixType.


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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281