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