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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


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



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

Yes I think the main problem is when a VectorType is used in an LLVM struct, then LLVM's vector alignment rules may impact the overall size.

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

I've moved the code to separate static functions. They don't have to be exposed in CodeGenFunction for now I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72281