[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()));
+})
+
----------------
fhahn wrote:
> 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 ?
You seem to have figured out the right place.  I don't think we've set anything up that lets us test TypeLocs right now.


================
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:
> > 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.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1781
+          Addr.getAlignment());
+    }
+  }
----------------
fhahn wrote:
> 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.
> 
Clang should already handle the possibility that the LLVM type is more or less aligned than the C type.  However, it does require the allocation size to match the C size, and since LLVM's vector alignment rules can affect vector sizes, I guess we do need to use an array instead.  This might be a little awkward for IRGen, though.

You should be able to assume that the type is right for the C type.

If you're thinking of ever adding interior padding, it might be reasonable to go ahead and extract functions to load/store these rather than growing those operations internally in EmitLoad/StoreOfScalar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281