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

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

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.

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.



================
Comment at: llvm/docs/LangRef.rst:15457
+loading of sub matrixes.  If <IsVolatile> is true, the intrinsic is considered
+a :ref:`volatile memory access <volatile>.`
 
----------------
Like `llvm.memcpy` and so on, you should document that the `align` attribute can be added to the pointer parameter to specify the required alignment.


================
Comment at: llvm/include/llvm/IR/MatrixBuilder.h:59
   /// \p Stride  - Space between columns
-  CallInst *CreateMatrixColumnwiseLoad(Value *DataPtr, unsigned Rows,
-                                       unsigned Columns, Value *Stride,
-                                       const Twine &Name = "") {
+  CallInst *CreateColumnMajorLoad(Value *DataPtr, unsigned Alignment,
+                                  Value *Stride, bool IsVolatile, unsigned Rows,
----------------
Alignment should be an `llvm::Align`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81472