[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:1024
+        Builder.CreateAlloca(Load->getType(), Load->getPointerAddressSpace());
+    Builder.CreateMemCpy(NewLd, MaybeAlign(NewLd->getAlignment()),
+                         Load->getPointerOperand(), Load->getAlign(),
----------------
LuoYuanke wrote:
> It seems the cost of Copy is not added to the cost model.
Yes currently the cost-model is kept simple, because we initially are focusing on bringing up the code-generation and are trying to make sure the matrix intrinsics are applicable to a wide range of uses cases. My current priority is progressing the clang patches, adding support for row-major matrixes and running the lowering pass by default on IR containing matrix intrinsics.

I've added a few additional TODOs to improve the cost-modeling. Currently we only automatically fuse operations, if we would run out of registers without fusion, so it only kicks in for matrixes sizes where the cost of coping should be negligible. But this should definitely be improved in the future. Not sure when we will get to it though and any contributions in that direction would be very welcome.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1034
+    // Adjust DT.
+    DTUpdates.push_back({DT.Insert, Fusion, Copy});
+    DTUpdates.push_back({DT.Insert, Copy, Check1});
----------------
LuoYuanke wrote:
> I'm confused about line 1034 and 1035. Should it be this? There is no edge from Fusion to Copy and from Copy to Check1.
> DTUpdates.push_back({DT.Insert, Copy, Fusion});
> DTUpdates.push_back({DT.Insert, Check1, Copy});
Right, those updates are not needed, which is much clearer after grouping all the update code together. It seems like the verification does not catch those unnecessary updates.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1106
+    IRBuilder<> Builder(Store);
+    for (unsigned J = 0; J < C; J += TileSize)
+      for (unsigned I = 0; I < R; I += TileSize) {
----------------
LuoYuanke wrote:
> It seems the tile size should be 2 dimension. 1 for row, and 1 for column.
Agreed, but as mentioned above I think using square tiles is a good compromise to bring up the infrastructure. After the initial commit, it should also be easier for other people to work on improving the tiling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75566