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

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

I have a few specific comments below but overall it would be great if we could simplify VisitBBFusion to avoid recursion and invalidating the iterator...

> This patch adds initial fusion for load/multiply/store chains of matrix
>  operations.
> 
> The patch contains roughly two parts:
> 
> Code generation for a fused load/multiply/store chain (LowerMatrixMultiplyFused).
>  First, we ensure that both loads of the multiply operands do not alias.

You mean any of the loads and the store?

> If they do, we create new non-aliasing copies of the operands. Note that this may introduce new basic block. Then we split the block containing the multiply at the multiply,

At the multiply or at the store?

> to simplify processing by returning the remainder of the original block to continue analysis (see 2.). Finally we process TileSize x TileSize blocks, that is, load tiles from the input operands, multiply and store them.
> 
> Identify fusion candidates.
>  To identify candidates for fusion, we look for @llvm.matrix.multiply with operands that are loads and a single use of the result in a store. To avoid generating unnecessary code for loads that later on get fused, we do a first pass over the function and only try fusing instructions, while keeping track of all other instructions with shape information in the function. We continue with the regular code generation for the remaining instructions with shape information after finishing fusion.



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


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:579
+      if (match(&Inst, m_Intrinsic<Intrinsic::matrix_multiply>())) {
+        if (BasicBlock *NextBB =
+                LowerMatrixMultiplyFused(cast<CallInst>(&Inst))) {
----------------
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?


================
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);
----------------
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?


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


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1124
+  //
+  /// No need to return LoweredMatrix since the single store user will be
+  /// lowered as part of this.
----------------
updated LoweredMatrix


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75566