[PATCH] D75566: [Matrix] Add initial tiling for load/multiply/store chains.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:572
 
+  /// Visit \b BB and try to fuse matrix instructions.
+  bool visitBBFusion(BasicBlock *BB,
----------------
anemet wrote:
> Document MatrixInsts
Code is gone.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:579
+      if (match(&Inst, m_Intrinsic<Intrinsic::matrix_multiply>())) {
+        if (BasicBlock *NextBB =
+                LowerMatrixMultiplyFused(cast<CallInst>(&Inst))) {
----------------
anemet wrote:
> The name NextBB is strange here.  It suggests that that is next one we are going to iterate to.  Sounds like this is more like a NewBB?
Code is gone.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:589-591
+      // Collect instructions producing matrix values, stores or bitcasts.
+      if (!Touched && ShapeMap.find(&Inst) != ShapeMap.end())
+        MatrixInsts.push_back(&Inst);
----------------
anemet wrote:
> Why is this necessary now?  We weren't doing this before during traversal.
> 
> When Touched is true we always return so I think Touched is always false here, no?
Code is gone.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:632
+    for (auto *BB : RPOT)
+      Changed |= visitBBFusion(BB, MatrixInsts);
+
----------------
anemet wrote:
> Needs comment on the logic here, what is returned in MatrixInsts, etc.
Code is gone.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1126
+  /// lowered as part of this.
+  BasicBlock *LowerMatrixMultiplyFused(CallInst *MatMul) {
+    if (!FuseMatrix)
----------------
anemet wrote:
> Document the BB returned
The return value is gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75566