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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to