[PATCH] D70897: [Matrix] Add forward shape propagation and first shape aware lowerings.

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

Also the existing test diffs are hard to read, please explain what's going on there.



================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:96-104
 /// LowerMatrixIntrinsics contains the methods used to lower matrix intrinsics.
 ///
 /// Currently, the lowering for each matrix intrinsic is done as follows:
 /// 1. Split the operand vectors containing an embedded matrix into a set of
 ///    column vectors, based on the shape information from the intrinsic.
 /// 2. Apply the transformation described by the intrinsic on the column
 ///    vectors, which yields a set of column vectors containing result matrix.
----------------
Needs an update explaining the shape propagation and its use.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:167
+      return NumRows != 0;
+    }
   };
----------------
Is this supposed to indicate empty or undefined?  I am not sure that the implicit conversion is self-explanatory here.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:192-193
   ///
   /// We split the flat vector \p MatrixVal containing a matrix with shape \p SI
   /// into column vectors.
   ColumnMatrixTy getMatrix(Value *MatrixVal, const ShapeInfo &SI,
----------------
Update comment


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:201-212
+    // Check if we lowered MatrixVal using shape information. In that case,
+    // return the existing column matrix.
+    auto Found = Inst2ColumnMatrix.find(MatrixVal);
+    if (Found != Inst2ColumnMatrix.end()) {
+      ColumnMatrixTy &M = Found->second;
+      if (SI.NumRows == M.getNumRows() && SI.NumColumns == M.getNumColumns())
+        return M;
----------------
The comment says you returning the ColumnMatrix here but you're not.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:257
+  }
+
+  /// Returns true if shape information can be used for \p V. The supported
----------------
I may be missing something but do these need the lambda?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:517
+  void finalizeLowering(Instruction *Inst, ColumnMatrixTy Matrix,
+                        IRBuilder<> &Builder) {
+    Inst2ColumnMatrix.insert(std::make_pair(Inst, Matrix));
----------------
Please add a comment of what's happening here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70897