[PATCH] D74172: [mlir][Linalg] Implement fusion of linalg.generic operation on tensors.

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:228
+               getInputs(),
+               [](Value v) { return v.getType().isa<RankedTensorType>(); }) &&
+           llvm::all_of(this->getOperation()->getResults(), [](Value v) {
----------------
nit: Can you separate the functor to avoid duplication?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:328
+  // Verify that the producer and consumer are ops on tensors.
+  if (!producer.hasTensorSemantics() || !consumer.hasTensorSemantics()) {
+    return false;
----------------
Please remove all trivial braces.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:374
+  auto t1 = producerArgIndexMap.compose(invProducerResultIndexMap);
+  // return is consumer loop -> producer arg tensor index, i.e. indexing_map for
+  // the producer argument in the fused operation.
----------------
Start sentences with a capital.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:403
+      producerOp.operand_begin(),
+      std::next(producerOp.operand_begin(), producerOp.getNumInputs()));
+
----------------
nit: you could use getOperands()[producerOp.getNumInputs()]


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:427
+  SmallVector<Type, 2> fusedOpResultTypes;
+  llvm::for_each(consumerOp.getResults(), [&fusedOpResultTypes](Value v) {
+    fusedOpResultTypes.push_back(v.getType());
----------------
This seems like an unnecessary usage of for_each. Why not just write a for loop? You don't even need to do that really. You could just use getResultTypes() directly as it is just an ArrayRef<Type>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74172