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