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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1274
+  TRY_TO(TraverseType(TL.getTypePtr()->getElementType()));
+})
+
----------------
Might as well do this instead of accumulating the technical debt.  MatrixTypeLoc should store the attribute location and the row and column expressions.


================
Comment at: clang/lib/AST/ASTContext.cpp:1945
+    Width = ElementInfo.Width * MT->getNumRows() * MT->getNumColumns();
+    Align = ElementInfo.Width;
+    break;
----------------
Should this be `ElementInfo.Align`?


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3349
+void CXXNameMangler::mangleType(const MatrixType *T) {
+  Out << "Dm" << T->getNumRows() << "_" << T->getNumColumns() << '_';
+  mangleType(T->getElementType());
----------------
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.


================
Comment at: clang/lib/Basic/Targets/OSTargets.cpp:138
+  if (Opts.EnableMatrix)
+    Builder.defineMacro("__MATRIX_EXTENSION__", "1");
 }
----------------
We're generally just using `__has_feature` checks these days instead of adding new predefined macros.  You can do that in `Features.def`.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2749
+  Subscripts.push_back(DBuilder.getOrCreateSubrange(0, Ty->getNumRows()));
+  llvm::DINodeArray SubscriptArray = DBuilder.getOrCreateArray(Subscripts);
+
----------------
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?


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1781
+          Addr.getAlignment());
+    }
+  }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281