[PATCH] D81472: [Matrix] Update load/store intrinsics.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 11 07:42:02 PDT 2020
fhahn added a comment.
In D81472#2086652 <https://reviews.llvm.org/D81472#2086652>, @rjmccall wrote:
> My immediate concern is just that I think the memory layout of the matrix should be orthogonal to the component layout of the vector. If you want the matrix intrinsics to support a variety of vector layouts, you should pass down the expected layout as a constant argument to the intrinsic rather than picking it up purely from whether the matrix is being loaded from a row-major or column-major layout in memory. I would guess that making that constant an i32 is probably sufficiently future-proof; if you ever need more structure than that, you'd probably be better off biting the bullet and adding an `llvm::MatrixType`.
Hm I understand the appeal of having a single very powerful intrinsic. Selecting the different variants by a single parameter is convenient in terms of maintaining backwards compatibility, but personally I find it more readable to include some of the variant information in the name. Of course there's a limit to the number of variants for which that approach is feasible. I think it is important to have this discussion, but I am not sure if it is in scope for this patch (which only adds a few smallish improvements to the naming/arguments of the intrinsics) and it might be better to discuss that once work on row-major versions of the intrinsics starts?
> My thinking here is that, for example, you're adding a builtin to Clang to do a column-major load. You want that to produce a column-major vector layout because that's your canonical layout within Clang. But you should also eventually add a builtin to do a row-major load, because there are quite a few reasons people might have a matrix in memory in row-major order: for example, if they've declared a nested C array, they'll naturally get row-major order. That builtin *also* needs to produce a column-major vector layout. So tying the two things together is bad intrinsic design.
Having intrinsics that can apply such conversions directly certainly is a convenient option here. But alternatively it should also be possible to have a small set of load/store intrinsics (e.g. load row-major from row-major, load column-major from column-major) and get the other variants by composing conversion functions (e.g. transpose (load row major() to load a row-major matrix from memory and convert it to column-major). Granted, I think a few other people also mentioned that they would prefer a few more powerful intrinsics, rather than having to compose intrinsics in earlier discussions.
In the end I am happy to go either way, as both approaches should be equivalent in terms of optimization power and it mostly boils down to slightly different matching in the lowering. But as I mentioned earlier, I think it would be good to make those changes once someone has time to add row-major support for the load/store intrinsics.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81472/new/
https://reviews.llvm.org/D81472
More information about the llvm-commits
mailing list