[PATCH] D75722: [mlir][Linalg] Implement padding for linalg.conv and lowering to loops.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:273
+                   OptionalAttr<I64ArrayAttr>:$dilations,
+                   OptionalAttr<I64ElementsAttr>:$padding);
 
----------------
This must be documented in great detail as it is very intrusive on a bunch of things.
You also want to go into all Linalg transformations and ensure that conv + dilations fails.
I will create a trait for that and refactor later.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:911
+    auto expr = op.getStride(i) * xs[i] + op.getDilation(i) * zs[i];
+    if (op.padding()) {
+      auto padLow = op.padding().getValue().getValue<IntegerAttr>({i, 0});
----------------
Please add a `// TODO(ntv): add a level of indirection to linalg.generic.`


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:185
+
+    ValueHandle zeroIndex = std_constant_index(0);
+    SmallVector<ValueHandle, 8> conds;
----------------
Please add a `// TODO(ntv): add a level of indirection to linalg.generic.`


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp:346
+  if (auto convOp = dyn_cast<linalg::ConvOp>(op.getOperation()))
+    assert(!convOp.padding() && "expected ConvOp doesn't has padding");
+
----------------
`llvm_unreachable("Unexpected conv with padding");`

here and everywhere in other transformations (tiling, fusion, promotion etc.)


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp:428
+  if (auto convOp = dyn_cast<linalg::ConvOp>(op.getOperation()))
+    assert(!convOp.padding() && "expected ConvOp doesn't has padding");
+
----------------
nit: doesn't have


================
Comment at: mlir/test/Dialect/Linalg/roundtrip.mlir:229
+  linalg.conv(%arg0, %arg1, %arg2) {dilations = [1, 1],
+                                    padding = dense<[[0, 1], [1, 1]]> : tensor<2x2xi64>,
+                                    strides = [1, 1]} :
----------------
the semantics of padding aren't clear to me, please document them in the op definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75722