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

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added a comment.

Some food for thought & perhaps you could add a test (or more) for FP types other than double?

-Gerolf



================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:14
+//  * Implement shape propagation
+//  * Implement optimizations to reduce shufflevector uses by using shape
+//    information.
----------------
Reduce or eliminate?


================
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.
----------------
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.


================
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) {
----------------
Always zero extend even when XBitWidth matches WidestBitWidth?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:71
+
+  // Distance to the desired column
+  // (column * + offset)
----------------
.. column = Col * Offset


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


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:73
+  // (column * + offset)
+  Value *ColumnOffset = Builder.CreateMul(Col, Offset);
+
----------------
ColumnOffset -> Distance? Offset seems a bit loaded.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:236
+    }
+
+    std::reverse(ToRemove.begin(), ToRemove.end());
----------------
You could early exit at this point (if (!Changed)....)


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:269
+
+    // Distance between start of one column and the start of the next
+    for (unsigned C = 0, E = Shape.NumColumns; C < E; ++C) {
----------------
Misplaced comment?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:303
+  /// Extract a column vector of \p NumElts starting at index (\p I, \p J) from
+  /// the matrix \p Cols represented as a vector of column vectors.
+  Value *extractVector(const ColumnMatrixTy &LM, unsigned I, unsigned J,
----------------
\p Cols -> \p  LM


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:322
+    Value *ExtendMask =
+        createSequentialMask(Builder, 0, BlockNumElts, NumElts - BlockNumElts);
+    Value *Undef = UndefValue::get(Block->getType());
----------------
assert(NumElts >= BlockNumElts)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70456