[PATCH] D133657: [Matrix] Optimize matrix transposes around additions

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:747
 
     // First sink all transposes inside matmuls, hoping that we end up with NN,
     // NT or TN variants.
----------------
Now also supports adds.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:832
+                [&](Value *T0, ShapeInfo Shape0, Value *T1, ShapeInfo Shape1) {
+                  auto *FAdd = cast<Instruction>(
+                      LocalBuilder.CreateFAdd(T0, T1, "mfadd"));
----------------
Could the lambda just return `Value *` instead of `Instruction *`? `setShapeInfo` and `ReplaceAllUsesWith` should work with `Value *`?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:849
 
     // If we have a TT matmul, lift the transpose.  We may be able to fold into
     // consuming multiply.
----------------
Now also handles TT additions.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:877
+        // A^t + B ^t -> (A + B)^t
+        else if (match(&I, m_FAdd(m_Value(A), m_Value(B))) &&
+                 match(A,
----------------
as the loop body gets bigger, it seems like it would be good to move this to a separate function to reduce the indentation level.

Same for the other loop above.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:882
+                 match(B,
+                       m_Intrinsic<Intrinsic::matrix_transpose>(
+                           m_Value(BT), m_ConstantInt(R), m_ConstantInt(C)))) {
----------------
Should we introduce a new `m_Transpose` helper to make this a bit more compact?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:891
+          ReplaceAllUsesWith(I, NewInst);
+          if (I.use_empty())
+            I.eraseFromParent();
----------------
This is the same as for the earlier if, right? Can we share this cleanup code? It's easy to miss


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133657