[PATCH] D73826: [mlir][Linalg] Provide a Tablegen backend to specify named Linalg ops declaratively

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

FYI:

  mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:795:22: warning: unused function 'verify' [-Wunused-function]
  static LogicalResult verify(FillOp op) {
                       ^
  1 warning generated.



In D73826#1876891 <https://reviews.llvm.org/D73826#1876891>, @jpienaar wrote:

> >> There is no disagreement that this may be useful for some cases: but we should strive to improve ODS itself to provide this facilities. I don't think we want dialect author to write Tablegen backends: this defeats the whole point of ODS in my opinion!
> > 
> > Well actually the new impl indeed changes to write a new TableGen backends. :) But I get your idea here. Yup I agree that we should strive to improve ODS itself and avoid dialect author to write TableGen expansion logic as much as possible. So features that apply to any dialect should be factored out and landed into main OpDefinitionGen. But there are cases it's inevitable like the Linalg case showed here: we have custom syntax defined for op definitions only reasonable to Linalg dialect. One can either completely roll its own TableGen backend or hook into OpDefinitionGen to avoid some boilerplate. I would argue the later saves developers' efforts and it's enhancing the functionality of ODS for dialect-specific use cases. :)
>
> I think a problem is that we have ODS now becoming a custom DSL for every dialect as every dialect can perform arbitrary interpretation and so there is no need to move towards common/reusable abstractions. ODS is restrictive for a reason :)


That's my initial position as well: the bar should be rather high to get into Tablegen backend, I'd rather not do this for every single "sugar" that can be added: at some point we'll have custom embedded parsers in Tablegen and create brand new languages...
That means there is a tradeoff and we'd like to make sure we consider and weigh a bit more the alternatives, including having something a bit more ugly in ODS if it avoid custom backends. Hence the current question in the diff to try to do this.

> The extension of mlir-tblgen from being ops defined in dialect independent manner & translate utilities for dialects, to custom DSL for dialect was snuck in and so while we have that landed, I think of that more as a bug that should be fixed instead rather than something we should point to as existance.

+1



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:263-264
 
+////////////////////////////////////////////////////////////////////////////////
+// Named Linalg ops, implemented as special configurations of generic ops.
+// At the moment these are not fully declarative special C++ code in the form of
----------------
the comment seems identical to line 55, but it is formatted like if it intends to open a new section


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:266
+// At the moment these are not fully declarative special C++ code in the form of
+// specifying the reference iterators and indexing maps.
+// This is necessary to obtain rank-polymorphic behavior and until a better
----------------
I don't parse this sentence


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:338-340
+  let input_indexing_maps = [AffineExpressions<["i", "r_j"]>,
+                             AffineExpressions<["r_j"]>];
+  let output_indexing_maps = [AffineExpressions<["i"]>];
----------------
jpienaar wrote:
> Just to be clear, so all of this is so that you can write
> 
>   let input_indexing_maps = [AffineExpressions<["i", "r_j"]>,
>                                AffineExpressions<["r_j"]>];
>   let output_indexing_maps = [AffineExpressions<["i"]>];
> 
> along with the op and from this you are generating 
> 
>   return SmallVector<AffineMap, 4>{AffineMap::get(2, 0, {i, j}),
>                                      AffineMap::get(2, 0, {j}),
>                                      AffineMap::get(2, 0, {i})};
> 
> ? 
I'm interested in the answer here, I'd like to make sure we understand the problem this is solving :)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73826