[PATCH] D86904: [mlir] Support for defining Types in tblgen
River Riddle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 1 20:03:39 PDT 2020
rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.
Sorry for the delay. Took another run through. Thanks.
================
Comment at: mlir/include/mlir/IR/OpBase.td:2364
+
+ // Short summary of the type
+ string summary = ?;
----------------
nit: Please add proper punctuation to the end of comments.
There are many cases within the revision still.
================
Comment at: mlir/include/mlir/IR/OpBase.td:2374
+ string storageNamespace = "detail";
+ // Should we generate the storage class? (Or use an existing one?)
+ bit genStorageClass = 1;
----------------
nit: Phrase the comments as statements instead of questions.
================
Comment at: mlir/include/mlir/IR/OpBase.td:2440
+ llvm::SmallVector<}] # arrayOf # [{, 4> tmpFields;
+ for (size_t i = 0, e = $_self.size(); i < e; i++) {
+ tmpFields.push_back($_self[i].allocateInto($_allocator));
----------------
nit: Drop trivial braces, and use pre-increment.
================
Comment at: mlir/include/mlir/IR/OpBase.td:2444
+ $_dst = $_allocator.copyInto(ArrayRef<}] # arrayOf # [{>(tmpFields));
+ }];
+}
----------------
The formatting is still a bit off.
================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:132
+template <typename T>
+void TypeDef::getParametersAs(SmallVectorImpl<T> ¶meters,
+ llvm::function_ref<T(TypeParameter)> map) const {
----------------
Seems like TypeDef could just provide a range of it's parameters and callers of this could just you llvm::map_range instead.
================
Comment at: mlir/lib/TableGen/TypeDef.cpp:69
+ size_t numParams = parametersDag->getNumArgs();
+ for (unsigned i = 0; i < numParams; i++) {
+ parameters.push_back(TypeParameter(parametersDag, i));
----------------
nit: Drop trivial braces here and throughout the revision.
================
Comment at: mlir/lib/TableGen/TypeDef.cpp:130
+ "Parameters DAG arguments must be either strings or defs "
+ "which inherit from TypeParameter\n");
+}
----------------
Drop the newline here.
================
Comment at: mlir/lib/TableGen/TypeDef.cpp:163
+ return getCppType();
+ } else {
+ llvm::errs() << "Parameters DAG arguments must be either strings or defs "
----------------
Don't use else after return.
================
Comment at: mlir/lib/TableGen/TypeDef.cpp:164
+ } else {
+ llvm::errs() << "Parameters DAG arguments must be either strings or defs "
+ "which inherit from TypeParameter\n";
----------------
Should this be a fatal error instead?
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:20
+
+namespace mlir {
+
----------------
Namespaces in source files should only really be used for class declarations. Everything else should be in the top-level scope.
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:28
+ if (parser.parseKeyword(&signStr))
+ return mlir::failure();
+ if (signStr.compare_lower("u") || signStr.compare_lower("unsigned"))
----------------
nit: Drop all of the `mlir::` in this file, and throughout the revision.
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:36
+ else {
+ parser.emitError(loc, "expected signed, unsigned, or none");
+ return mlir::failure();
----------------
nit: Return the error directly.
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:63
+ SmallVector<int, 4> arrayOfInts;
+ if (parser.parseLess())
+ return Type();
----------------
nit: Merge all of these together using `||`
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:93
+ auto intArray = getArrayOfInts();
+ for (size_t idx = 0; idx < intArray.size(); idx++) {
+ printer << intArray[idx];
----------------
nit: Use llvm::interleaveComma here instead.
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:101
+
+bool operator==(const FieldInfo &a, const FieldInfo &b) {
+ return a.name == b.name && a.type == b.type;
----------------
The definitions of methods in header files should be fully qualified `::` and not placed in a namespace in the .cpp.
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.h:27
+struct FieldInfo {
+public:
----------------
Please add comments for this class and it's usages.
================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.h:28
+struct FieldInfo {
+public:
+ StringRef name;
----------------
This public seems unnecessary.
================
Comment at: mlir/tools/mlir-tblgen/OpDocGen.cpp:202
+ << "`\n";
+ } else {
+ os << "\nSyntax:\n\n```\n!" << td.getDialect().getName() << "."
----------------
nit: Prefer early return instead.
================
Comment at: mlir/tools/mlir-tblgen/OpDocGen.cpp:205
+ << td.getMnemonic() << "<\n";
+ for (auto *it = parameters.begin(); it < parameters.end(); it++) {
+ os << " " << it->getSyntax();
----------------
nit: Unless the end loop iterator changes, cache it.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:83
+ SmallVector<std::string, 4> parameters;
+ if (prependComma)
+ parameters.push_back("");
----------------
The name of this parameter doesn't seem to match what you are doing here.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:85
+ parameters.push_back("");
+ typeDef.getParametersAs<std::string>(parameters, [](auto parameter) {
+ return (parameter.getCppType() + " " + parameter.getName()).str();
----------------
Could you just format the parameters into a llvm::raw_string_ostream instead of constructing separate strings?
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:98
+static std::string constructParametersInitializers(TypeDef &typeDef) {
+ SmallVector<std::string, 4> parameters;
+ typeDef.getParametersAs<std::string>(parameters, [](auto parameter) {
----------------
Same comment as above.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:134
+ {1}::{2}> {{
+public:
+ /// Inherit some necessary constructors from 'TypeBase'.
----------------
This looks like the indent is off.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:186
+ if (auto mnenomic = typeDef.getMnemonic()) {
+ os << " static ::llvm::StringRef getMnemonic() { return \"" << mnenomic
+ << "\"; }\n";
----------------
Do we need to generate this method? Seems like the uses of it by the generator could just inline the string instead.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:197
+
+ for (auto parameter : parameters) {
+ SmallString<16> name = parameter.getName();
----------------
nit: Spell out auto here, also use a reference instead of by-value.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:222
+ // well known print/parse dispatch function declarations
+ os << " ::mlir::Type generatedTypeParser(::mlir::MLIRContext* ctxt, "
+ "::mlir::DialectAsmParser& parser, ::llvm::StringRef mnenomic);\n";
----------------
Seems like this would be something only generated in the source file and marked as static.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:274
+ struct {1} : public ::mlir::TypeStorage {{
+ {1} ({2})
+ : {3} {{ }
----------------
This looks over indented, should be 2 spaces not 4.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:293
+ static {0} *construct(::mlir::TypeStorageAllocator &allocator,
+ const KeyTy &key) {{
+)";
----------------
This looks misaligned.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:311
+ auto fmtCtxt = FmtContext().addSubst("_allocator", "allocator");
+ for (auto parameter : parameters) {
+ auto allocCode = parameter.getAllocator();
----------------
nit: Spell out auto here and don't use a by-value iterator variable.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:348
+ for (size_t i = 0; i < parameters.size(); i++) {
+ os << llvm::formatv(" auto {0} = std::get<{1}>(key);\n",
+ parameters[i].getName(), i);
----------------
Should these be `auto &`instead?
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:373
+ typeDef.getStorageClassName());
+ // I want C++17's unboxing!!!
+ for (size_t i = 0; i < parameters.size(); i++) {
----------------
Please remove all of the personal comments from the revision.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:381
+ return mlir::failure();
+ // return an allocated copy
+ os << llvm::formatv(typeDefStorageClassConstructorReturn,
----------------
nit: Comments should be complete sentences. This applies throughout the revision.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:420
+ if (typeDef.genAccessors()) {
+ for (auto parameter : parameters) {
+ SmallString<16> name = parameter.getName();
----------------
nit: Same comment about auto here as above.
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:481
+
+ } // typeDef.getMnemonic()
+ return mlir::success();
----------------
Stray comment?
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:505
+
+ os << "bool generatedTypePrinter(::mlir::Type type, "
+ "::mlir::DialectAsmPrinter& "
----------------
Can we use a LogicalResult return instead?
================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:513
+ os << llvm::formatv(" .Case<{0}::{1}>([&](::mlir::Type t) {{ "
+ "t.dyn_cast<{0}::{1}>().print(printer); })\n",
+ typeDef.getDialect().getCppNamespace(),
----------------
Can we avoid generating print/parse on the types themselves? We should try to keep as much internal details hidden away as possible.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86904/new/
https://reviews.llvm.org/D86904
More information about the llvm-commits
mailing list