[PATCH] D73405: [mlir] Add initial support for parsing a declarative operation assembly format
Jacques Pienaar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 27 21:32:42 PST 2020
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
More information about the llvm-commits
mailing list