[PATCH] D79468: [mlir][Linalg] Start a LinalgToStandard pass and move conversion to library calls.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

I think you need to relax the semantics of `memref_cast` first. It currently says (emphasis mine)

> The memref_cast operation converts a memref from one type to an equivalent type with a compatible shape. The source and destination types are compatible if: a. both are ranked memref types with the same element type, **affine mappings**, address space, and rank but where the individual dimensions may add or remove constant dimensions from the memref type.

so the new uses introduced here and the their lowering directly contradict what is stated in the doc. (I am surprised we don't have a verifier to check it). I am not fundamentally opposed to such relaxation, but we may want to consider introducing a separate, more dangerous cast operation, similarly to `static_cast`/`reinterpret_cast` different in C++. The layout-changing cast may create out-of-bounds accesses whereas static/dynamic info cast will never do that.



================
Comment at: mlir/lib/Conversion/LinalgToStandard/LinalgToStandard.cpp:20
+
+template <typename LinalgOp>
+static SmallVector<Type, 4> extractOperandTypes(Operation *op) {
----------------
Document this plz


================
Comment at: mlir/lib/Conversion/LinalgToStandard/LinalgToStandard.cpp:46
+  result.reserve(numLoops + op->getNumOperands());
+  for (unsigned i = 0; i < numLoops; ++i)
+    result.push_back(IndexType::get(ctx));
----------------
`result.resize(numLoops, IndexType::get(ctx));` spares you a loop


================
Comment at: mlir/lib/Conversion/LinalgToStandard/LinalgToStandard.cpp:104
+    }
+    res.push_back(b.create<MemRefCastOp>(
+        loc,
----------------
How about storing the result of `b.create` in a variable and then pushing it back?  It will likely be less LoC and more readable at the same time.


================
Comment at: mlir/lib/Conversion/LinalgToStandard/LinalgToStandard.cpp:106
+        loc,
+        MemRefType::get(memrefType.getShape(), memrefType.getElementType(), {},
+                        memrefType.getMemorySpace()),
----------------
I don't recall memref_cast, or its lowering, to support layout changes. It can add or remove static information, but a memref without an explicit layout map just has an implicit row-major layout. Did you mean to cast it to unranked memref maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79468