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

[Jacques Pienaar via Phabricator via llvm-commits]

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

Nice, thanks

One clang-format failure



================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:130
+
+/// This class represents the `functional-type` directive.
+struct FunctionalTypeDirective
----------------
Could we also add what these means? (and/or add an example)


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:224-225
+  std::vector<Optional<int>> buildableOperandTypes, buildableResultTypes;
+  /// An optional resolver for all operand or result types. If this is set, the
+  /// above is empty.
+  bool allOperandTypes, allResultTypes;
----------------
Perhaps hoist this above and specify that one could either specify 1) general specification, or 2) a per operand/result type ? Else I have to read one comment further to know when the previous would be empty.

Lets also expand std::vector<Optional<int>> part, it is not immediately obvious if the size of the vector match the number of operands/results or what the optional int represents.


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:250
+    // Keywords.
+    keyword_start,
+    kw_attr_dict,
----------------
Why is keyword spelled out here and in end, but abbreviated elsewhere?


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:606
+  auto findArg = [&](auto &&range) {
+    auto it = llvm::find_if(range, [=](auto &arg) { return arg.name == name; });
+    return it != range.end() ? &*it : nullptr;
----------------
Is 'name' the only value referenced inside lambda?


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:696
+  std::unique_ptr<Element> inputs, results;
+  if (failed(parseToken(Token::l_paren, "expected '(' before argument list")) ||
+      failed(parseTypeDirectiveOperand(inputs)) ||
----------------
With these all within one file, having a simple parsing return using bool & using shortcut logic would read easier (E.g., I'm not really convinced LogicalResult helps much here, I feel LogicalResult obscures the parsing logic and doesn't add much value/verification as it is all self-sufficient in this file).



================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:723
+  if (isTopLevel)
+    return emitError(loc, "'results' directive can *not* be used as a "
+                          "top-level directive");
----------------
s/*not*/not/ , additional emphasis not needed in these error messages.


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:765
+      return emitError(loc, "'type' of '" + var->getVar()->name +
+                                "' is already covered");
+    seenResultTypes.set(resIdx);
----------------
s/covered/bound/ ? Or defined ?


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:791-797
+  const llvm::Record &opDef = op.getDef();
+  if (!hasStringAttribute(opDef, "assemblyFormat"))
+    return;
+
+  StringRef formatStr = opDef.getValueAsString("assemblyFormat");
+  if (formatStr.empty())
+    return;
----------------
You could combine these two and update the comment (verification on a valid format field is below, these just determines if there is an assembly format specified).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73405