[PATCH] D154363: [WIP][mlir] Add an interface to decompose complex ops

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/include/mlir/Interfaces/AggregatedOpInterface.td:2
+//===- TilingInterface.td - Interface for tiling operations *- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
nicolasvasilache wrote:
> chelini wrote:
> > Why this interface is not part of Linalg? 
> +1 to moving the interface in Linalg, this is not a general concept AFAICT
I thought this could generally be useful.
E.g., we have some expand passes (`expand-strided-metadata`, `memref-expand`) right now that could fit in this interface. But yeah, let's leave that out.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:2363
+  assert(indexingMaps.size() == 2 && "We should one map for each input");
+  assert(indexingMaps[0].isIdentity() &&
+         "All parallel should give us identity map for input");
----------------
chelini wrote:
> I would expect to have identity map for both input operands. Is this not the case?
No, the second map drops one dimension since the max performed a reduction on `dim`


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:2382
+/// }
+static Value computeSoftmax(Value numerator, Value denominator, Value output,
+                            int64_t dim, Location loc, OpBuilder &builder) {
----------------
chelini wrote:
> nit: I think naming is a bit misleading here. Simply, `buildDivOp`?
Good point.


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:2444
+  // Step 2: Subtract max from input and exponentiate.
+  Value numerator = subtractAndExp(input, max, outputNd, reductionDim, loc, b);
+
----------------
chelini wrote:
> nit: I would rename: `subtractAndExp` to `buildSubAndExpOp`.
👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154363