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

[Jacques Pienaar via Phabricator via llvm-commits]

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

In D73826#1874647 <https://reviews.llvm.org/D73826#1874647>, @antiagainst wrote:

> In D73826#1873559 <https://reviews.llvm.org/D73826#1873559>, @mehdi_amini wrote:
>
> > In D73826#1865623 <https://reviews.llvm.org/D73826#1865623>, @antiagainst wrote:
> >
> > > The main value I see with the OpDefinitionGen hook is that it allows one to **automatically** insert traits and methods into the generated C++ op classes. This is preferable when a dialect's ops have **a large variation of potential traits and methods**, and that information is already encoded with other fields in the op definitions
> >
> >
> > 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 :)

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.

In D73826#1863982 <https://reviews.llvm.org/D73826#1863982>, @nicolasvasilache wrote:

> @jpienaar @mehdi_amini !pingplease.
>
> > The above functionality could already be handled with OpInterfaces + the way export generation is handled for TFLite dialect [which doesn't use OpInterfaces as that predated it, but could], one need not change OpDefinitionGen to get this behavior.
>
> This is not actionable for me, I do not see how to declaratively express AffineMaps in a declarative fashion in Tablegen and generating the proper OpInterface methods without the OpDefinitionGen hook.


This change still does not do that, you do not have a way of specifying AffineMaps, you allow some subclass of them in a new format way and in a way that is dialect specific. I think this makes using ODS as Linalg DSL easier, but I'm not sure even there the added complexity is work it (and you could do this just using basic string interpolation locally with the same amount of portability and analyzability as you are providing here, nothing else in the system is gaining much from this and whether you had it in TableGen  itself or as generator post seem to provide equal benefit here and if general patterns arose from which dialect indepent support could be added we could refactor).



================
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"]>];
----------------
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})};

? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73826