[PATCH] D76205: Add support for custom op parser/printer hooks to know about result names.

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Sorry for the delay. Looks good, just some nits



================
Comment at: mlir/include/mlir/IR/OpImplementation.h:250
+  /// Return the name of the specified result in the specified syntax, as well
+  /// as the subelement in the name.  It returns an empty string an ~0U for
+  /// invalid result numbers.  For example, in this operation:
----------------
nit: sub-element


================
Comment at: mlir/include/mlir/IR/OpImplementation.h:250
+  /// Return the name of the specified result in the specified syntax, as well
+  /// as the subelement in the name.  It returns an empty string an ~0U for
+  /// invalid result numbers.  For example, in this operation:
----------------
rriddle wrote:
> nit: sub-element
typo: `empty string and`


================
Comment at: mlir/include/mlir/IR/OpImplementation.h:264
+  /// Return the number of declared SSA results.  This returns 4 for the foo.op
+  /// example in the comment for getResultName.
+  virtual size_t getNumResults() const = 0;
----------------
nit: `getResultName`


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1994
+    printValueID(value, /*printResultNo=*/true, &os);
+  };
 
----------------
nit: Remove the semi-colon here.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:2206
+    streamOverride = &os;
+  state->getSSANameState().printValueID(value, printResultNo, *streamOverride);
 }
----------------
nit: is it cleaner to just inline as `streamOverride ? streamOverride : &os`?


================
Comment at: mlir/lib/Parser/Parser.cpp:4019
+        // Don't pass on the leading %.
+        auto name = std::get<0>(entry).drop_front();
+        return {name, resultNo};
----------------
nit: auto -> StringRef


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:399
+// This op has fancy handling of its SSA result name.
+
+static ParseResult parseStringAttrPrettyNameOp(OpAsmParser &parser,
----------------
nit: Remove the extra newline here.


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:412
+  bool hadNames = false;
+  for (auto &attr : result.attributes) {
+    if (attr.first.is("names")) {
----------------
`llvm::any_of(result.attributes, [](NamedAttribute attr) { return attr.first.is("names"); })`
?


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:420
+  // If there was no name specified, check to see if there was a useful name
+  // specified in the asm file.
+  if (!hadNames && parser.getNumResults()) {
----------------
nit: Can we early exit here instead?


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:422
+  if (!hadNames && parser.getNumResults()) {
+    SmallVector<Attribute, 4> names;
+    auto *context = result.getContext();
----------------
You could collect a vector of StringRefs and use `Builder::getStrArrayAttr`


================
Comment at: mlir/test/lib/TestDialect/TestDialect.cpp:428
+      StringRef nameStr;
+      if (!resultName.first.empty() && !isdigit(resultName.first[0])) {
+        nameStr = resultName.first;
----------------
nit: Drop trivial braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76205