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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
