fhahn added inline comments.
================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1274
+ TRY_TO(TraverseType(TL.getTypePtr()->getElementType()));
+})
+
----------------
rjmccall wrote:
> Might as well do this instead of accumulating the technical debt.
> MatrixTypeLoc should store the attribute location and the row and column
> expressions.
I tried adding a proper MatrixTypeLoc. For the regular MatrixType, I am not
sure where the Row/Column expression should actually be initialized though.
Also, what would be a good place for testing the TypeLocs ?
================
Comment at: clang/lib/AST/ASTContext.cpp:1945
+ Width = ElementInfo.Width * MT->getNumRows() * MT->getNumColumns();
+ Align = ElementInfo.Width;
+ break;
----------------
rjmccall wrote:
> Should this be `ElementInfo.Align`?
Yes!
================
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:
> 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.
================
Comment at: clang/lib/Basic/Targets/OSTargets.cpp:138
+ if (Opts.EnableMatrix)
+ Builder.defineMacro("__MATRIX_EXTENSION__", "1");
}
----------------
rjmccall wrote:
> We're generally just using `__has_feature` checks these days instead of
> adding new predefined macros. You can do that in `Features.def`.
Thanks, I've updated the code to use that and added a parser test with the
feature disabled as well.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2749
+ Subscripts.push_back(DBuilder.getOrCreateSubrange(0, Ty->getNumRows()));
+ llvm::DINodeArray SubscriptArray = DBuilder.getOrCreateArray(Subscripts);
+
----------------
rjmccall wrote:
> Is this actually the same DWARF type as would be created by a nested C array
> type, or is it a two-dimensional array type?
I think it should create the same DWARF type as for a nested ArrayType (e.g. a
2 x 3 float matrix will have the same DWARF type as a float x[3][2] array).
I've added a test to check the generation of debug info.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1781
+ Addr.getAlignment());
+ }
+ }
----------------
rjmccall wrote:
> Please use `CreateElementBitCast`. It's cleaner and will handle address
> spaces correctly.
>
> What are you trying to do here? You're expecting the pointer to be a pointer
> to an array, and you're casting that to a pointer to a vector of the same
> number of elements? Why not just use the vector type as the expected memory
> type for the matrix type?
> What are you trying to do here? You're expecting the pointer to be a pointer
> to an array, and you're casting that to a pointer to a vector of the same
> number of elements? Why not just use the vector type as the expected memory
> type for the matrix type?
The main reason for using an array type as expected memory type is to avoid
having to use LLVM's large vector alignment for matrix values as class/struct
members. Consistently using pointers to arrays as expected memory type seemed
the least ugly way to handle it, but I am probably missing a much better
alternative.
An alternative could be to use packed LLVM structs with matrix members, but
that seems more invasive.
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
