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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:403
+      producerOp.operand_begin(),
+      std::next(producerOp.operand_begin(), producerOp.getNumInputs()));
+
----------------
rriddle wrote:
> nit: you could use getOperands()[producerOp.getNumInputs()]
I am not sure about that. I am inserting a range of elements. I think the insert method used needs an iterator for the begin/end.


================
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());
----------------
rriddle wrote:
> 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>
Thanks for getResultTypes() (cant keep all the utility methods in my head, but should have checked)


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:346
+  // must match the number of loops of the producer.
+  AffineMap consumerIndexMap = consumerOp.getIndexingMap(consumerIdx);
+  if (consumerIndexMap.getNumResults() != producerOp.getNumLoops()) {
----------------
nicolasvasilache wrote:
> re. AffineMap, note that the current "inversion" procedure is best effort atm and will fail in many cases.
> You want to check that the inverse computation succeeeds and return false if not.
> Otherwise I suspect you'll crash later.
> 
> Example that I think should fail today and may crash your transformation down the line: `(i, j) -> (i, i+j)`
Thanks. Forgot to check for permutation. Added that check now. I think that is enough for now. If we need to support check of invertability, we can extend that later.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp:411
+                               consumerOp.indexing_maps().end());
+  fusedIndexingMapAttrs.erase(
+      std::next(fusedIndexingMapAttrs.begin(), consumerIdx));
----------------
nicolasvasilache wrote:
> Haven't checked deeply that you don't have an off by 1 here somewhere, please be extra careful but you know better than me :)
I did hit the off by one. Dont need to worry at erase time. THe first insertion will be fine too. Only need to increment the insert position after each insertion to make sure things are inserted at the right place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74172