[PATCH] D73405: [mlir] Add initial support for parsing a declarative operation assembly format

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked an inline comment as done.
antiagainst added a comment.

This is super awesome! The code is well written so not many comments from me. Thanks River! One high-level comment: can we add document for this? Doing it as a following commit is fine.



================
Comment at: mlir/include/mlir/IR/OpBase.td:1575
+  // Custom format.
+  string format = ?;
+
----------------
`format` itself is a bit too broad and can be confusing. What about naming it as something like `asmForm` or `assemblyForm`? This applies to other places; I see you generally use "declarative op format". I think calling it "declarative op assembly form" is more natural and descriptive. WDYT?


================
Comment at: mlir/test/mlir-tblgen/op-format-spec.td:1
+// RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s 2>&1 | FileCheck %s
+// --dump-input-on-failure
----------------
Awesome tests!!! :) 


================
Comment at: mlir/test/mlir-tblgen/op-format-spec.td:2
+// RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s 2>&1 | FileCheck %s
+// --dump-input-on-failure
+
----------------
Accidentally breaking the lines? I'm not sure this is gonna work. :)


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:125
+/// This class represents the `functional-type` directive.
+struct FunctionalTypeDirective
+    : public DirectiveElement<Element::Kind::FunctionalTypeDirective> {
----------------
Nit: Add empty lines to make it more readable?



================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:408
+  // Otherwise, consume the rest of the characters.
+  while (isalpha(*curPtr) || isdigit(*curPtr) || *curPtr == '_')
+    ++curPtr;
----------------
This set is common enough. What about making it a utility function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73405