[PATCH] D77067: [mlir][Linalg] Create a tool to generate named Linalg ops from a Tensor Comprehensions-like specification.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 20 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:698
+
+def NamedStructuredOpTraits : NativeOpTrait<"linalg::NamedStructuredOpTraits">;
+
----------------
silvas wrote:
> Is there another diff that includes this?
This is eagerly included to allow the test to pipe through mlir-tblgen and verify the ODS is well-formed (as was suggested by @ftynse, see other comment). The companion revision https://reviews.llvm.org/D76456 does the plumbing assuming a parser exists and shows how to make this run end-to-end.

In the current form this is non-functional and only exists for the purpose of verifying well-formedness and avoiding a giant diff when things can be (reasonably well) separated.  

If you have strong objections against this interim state, I would rather drop the piping through tablegen rather than merge revisions (but @ftynse may have his own objections to this).


================
Comment at: mlir/test/mlir-linalg-ods-gen/test-linalg-ods-gen.tc:1
+// RUN: mlir-linalg-ods-gen %s -gen-ods-decl=1 | FileCheck %s --check-prefix=ODS
+
----------------
silvas wrote:
> add test that exercises multiple comprehensions in the body
Despite the parser accepting it, there is no support for that atm and some eperiment + design is required here.
Emitting an error for now.


================
Comment at: mlir/test/mlir-linalg-ods-gen/test-linalg-ods-gen.tc:5
+
+// RUN: mlir-linalg-ods-gen %s -gen-ods-decl=1 -o %t \
+// RUN: && cat %S/../../include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td %t \
----------------
silvas wrote:
> layering-wise would prefer to not test this here. If needed, we can add a separate test elsewhere that does this .td -> .inc file check. Strictly speaking what ends up in the .inc file is not really the concern of this component, only the contents of the .td file.
> 
This was suggested by @ftynse to show the ODS is valid and how it connects to tblgen by mirroring this test: https://github.com/llvm/llvm-project/blob/master/mlir/test/mlir-tblgen/llvm-intrinsics.td#L11.

I am fine either way, would just like consensus on this before reverting back to the previous state.
Please reopen if you feel strongly about this.
@ftynse any strong opinion?


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1379
+
+  // 4. Print.
+  if (genODSDecl) {
----------------
silvas wrote:
> How can we emit ODS before we finish processing the whole `tc-def` production?
Thanks!


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1454
+  if (failed(
+          parser.parseCommaSeparatedListUntil(Token::r_brace, parseOne, false)))
+    return failure();
----------------
silvas wrote:
> comma separated comprehensions seems to contradict the grammar?
you're right, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77067