[PATCH] D81472: [Matrix] Update load/store intrinsics.

[Florian Hahn via Phabricator via llvm-commits]

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