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

[Florian Hahn via Phabricator via cfe-commits]

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