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

Wed Jun 10 08:12:25 PDT 2020

fhahn updated this revision to Diff 269855.
fhahn added a comment.

In D81472#2083075 <https://reviews.llvm.org/D81472#2083075>, @rjmccall wrote:

> I like the name change, although I wonder if you could just have a single intrinsic that takes both a row stride and a column stride and recognizes the common patterns.  Presumably even with column-major ordering you already want to optimize the case where the stride is a constant equal to the row count, so this would just be a generalization of that.

I am not sure about having a single intrinsic. The `column.major` part in the name signifies that the resulting matrix is in column-major layout (whic, but  is then used internally during he lowering). I suppose it would be possible to have both row & column strides and use a special value to indicate what the leading dimension is, but it seems to me that having dedicated intrinsics would be more explicit.

> Are you planning to actually support alignment in a follow-up patch?  I don't see anything here that propagates alignment to the lowering routines.

Yes, this patch just adjusts the intrinsics definitions/langref. Respecting both `IsVolatile` & the alignment attribute will be done in follow-up patches. There's already one for `IsVolatile` D81498 <https://reviews.llvm.org/D81498>. I think setting the alignment correctly for the split stores is not completely trivial, because the original alignment will hold for the first split access, but may not hold for the subsequent accesses and some extra work is needed to figure out which alignments to use for subsequent stores, depending on the stride.

In D81472#2082756 <https://reviews.llvm.org/D81472#2082756>, @braedy wrote:

> I admittedly don't know Clang's naming scheme, so feel free to ignore this, but I dislike the change to `*.column.major.*` because it feels like both are changeable parameters (i.e. column/row major/minor are two different axes).  To anyone who knows what they're looking for they would understand that this doesn't make any sense, but the potential remains. I'm not sure that dropping the major portion makes any sense either because it might seem like you're promising loads and stores no matter the storage order

I am not sure I quite follow I am afraid. The `column major` in the name is meant to refer to what data layout is assumed for the operation. The intrinsics treat both the accessed memory as well as the loaded/stored value as column-major. It could also be a parameter, but the layout is intentionally encoded in the name.  We have to use `column.major.` instead of something like `.column_major.`, because of the way intrinsic names are handled: `_` are automatically replaced by `.`.  In the future, we are planning on adding `llvm.matrix.row.major.load/store`.

So far, I am not planning on adding variants that allow the memory-layout to not match the operand/result layout (e.g. treat memory as row-major but the loaded value is in column-major).

>   (e.g. `llvm.matrix.column.load` might seem like it promises a column load even if the matrix is row major, which isn't the intent). Removing the `.` between them is an option but then it's not that much different from the original `columnwise`, unless "in line with Clang's naming" means using "column major" instead of "column-wise". Maybe I'm just putting too much meaning into the `.` too, but I'd rather mention it and be told it's fine than not say anything :)

Thanks for sharing your thoughts! As mentioned above, we have to use either  `.` as separator or no separator at all. Please let me know if that makes sense

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81472

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/include/llvm/IR/MatrixBuilder.h
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
  llvm/test/Transforms/LowerMatrixIntrinsics/bigger-expressions-double.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/const-gep.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/multiply-add-sub-double-row-major.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/propagate-backward.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/propagate-forward.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/propagate-mixed-users.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/propagate-multiple-iterations.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/remarks-inlining.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/remarks.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-load-double.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-load-float.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-load-i32.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-store-double.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-store-float.ll
  llvm/test/Transforms/LowerMatrixIntrinsics/strided-store-i32.ll
  llvm/test/Verifier/matrix-intrinsics.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81472.269855.patch
Type: text/x-patch
Size: 98405 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200610/39036e91/attachment.bin>