[PATCH] D70456: [Matrix] Add first set of matrix intrinsics and initial lowering pass.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:44
+// compute the address of the element at index \p Row, \p Col.  \p Offset is
+// the number of elements to skip to move same row, next column (this the
+// number of rows other than accessing a submatrix.
----------------
Gerolf wrote:
> Would this be clearer: \p Offset is usually the number of elements in a column (equivalent to the number of rows of the matrix). When MatrixPtr represents a submatrix, Offset is the number of rows of the parent matrix, which is the matrix that contains the submatrix? At least "other than accessing a sub matrix." needs consideration.
I've updated the wording, hopefully it is clearer now.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:62
+  // If they are not the same width, extend all to be the same width
+  if (RowBitWidth != WidestBitWidth || ColBitWidth != WidestBitWidth ||
+      OffsetBitWidth != WidestBitWidth) {
----------------
Gerolf wrote:
> Always zero extend even when XBitWidth matches WidestBitWidth?
The intrinsics now take i32 only, so we do not need this any longer.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:72
+  // Distance to the desired column
+  // (column * + offset)
+  Value *ColumnOffset = Builder.CreateMul(Col, Offset);
----------------
Gerolf wrote:
> Then you could remove this line?!
I've compacted the comments a bit.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:236
+    }
+
+    std::reverse(ToRemove.begin(), ToRemove.end());
----------------
Gerolf wrote:
> You could early exit at this point (if (!Changed)....)
We do not really need to keep a list of instructions to remove currently. We can just remove them directly. I've dropped the code for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70456