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

[Sean Silva via Phabricator via llvm-commits]

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

First round of comments.



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td:698
+
+def NamedStructuredOpTraits : NativeOpTrait<"linalg::NamedStructuredOpTraits">;
+
----------------
Is there another diff that includes 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
+
----------------
add test that exercises multiple comprehensions in the body


================
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 \
----------------
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.



================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:974
+  ///
+  ///   tc-def ::= `def` tc-id `(`tensor-def-list`)` `->` `(` tensor-def-list`)`
+  ///     `{` comprehension-list `}`
----------------
some spaces needed around first tensor-def-list


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:977
+  ///
+  /// All the affine-expr in a `tensor-typedef` must be dimensionless (i.e.
+  /// contain only expressions involving symbols and constants), but can
----------------
I don't see affine-expr or tensor-typedef mentioned locally in this comment. move this to the appropriate comment?


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1370
+          LLVM_DEBUG(llvm::dbgs() << "\nexisting: " << tensor.indexingMap);
+          assert(false &&
+                 "Unexpected multi-read of a tensor with different accesses");
----------------
should this be a diagnostic?


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


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1410
+/// otherwise contain arbitrary affine expressions.
+LogicalResult TCParser::processTensorComprehension(llvm::raw_ostream &os) {
+  if (failed(parser.parseToken(Token::kw_def, "expected 'def' to define a TC")))
----------------
maybe rename to "parseAndEmitTCDef"? Also probably rename processOneComprehension to parseAndEmitOneComprehension to be consistent with that.


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1428
+  if (failed(parser.parseToken(Token::minus, "expected '->'")) ||
+      failed(parser.parseToken(Token::gt, "expected '->'")) ||
+      failed(parser.parseToken(Token::l_paren, "expected '('")))
----------------
typo in the "expected" string.


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1438-1439
+
+  // At this point, all tensors have been parsed. Resize all AffineMaps to
+  // normalize the number of eagerly discovered symbols.
+  for (auto &tensor : tensors) {
----------------
Can you make this comment a bit easier to understand. What is an "eagerly discovered symbol" and how does this "normalize" it?


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1444-1445
+        map.isEmpty()
+            ? AffineMap::get(/*dimCount=*/0, symbols.size(), parser.context)
+            : AffineMap::get(/*dimCount=*/0, symbols.size(), map.getResults());
+  }
----------------
Instead of the ternary, use
```
 static AffineMap get(unsigned dimCount, unsigned symbolCount,
                       ArrayRef<AffineExpr> results, MLIRContext *context);
```


================
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();
----------------
comma separated comprehensions seems to contradict the grammar?


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1655
+  std::string errorMessage;
+  auto file = mlir::openInputFile(inputFilename, &errorMessage);
+  if (!file) {
----------------
auto here obscures things IMO


================
Comment at: mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-gen.cpp:1661
+
+  auto output = openOutputFile(outputFilename, &errorMessage);
+  if (!output) {
----------------
auto here obscures things IMO


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77067