[PATCH] D86904: [mlir] Support for defining Types in tblgen

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rdzhabarov.

In D86904#2323208 <https://reviews.llvm.org/D86904#2323208>, @jdd wrote:

> I'll update the documentation as you requested. Do you mind if I do it in a subsequent commit? I have a feeling there's going to be some back-and-forth on the documentation and I don't want that editorial process to gate this commit.

That's fine with me.

LGTM after fixing the remaining comments.



================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:71
+  // supposed to auto-generate them.
+  llvm::Optional<StringRef> getMnemonic() const;
+
----------------
jdd wrote:
> rriddle wrote:
> > nit: Can we remove the llvm:: on these as well?
> Actually, no. It doesn't find Optional if I remove it. I'd have to add a 'using namespace' in the header, which is a no-no.
> 
> Why StringRef and ArrayRef work and nothing else in the llvm namespace is a mystery to me, though my understanding of C++ namespace scoping rules is doubtlessly lacking.
Optional, like ArrayRef/StringRef/and many other things, is already re-exported into the MLIR namespace. 

https://github.com/llvm/llvm-project/blob/3f2386de6325157324a0dc2ae00ef3db3a144563/mlir/include/mlir/Support/LLVM.h#L110


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:87
+
+namespace mlir {
+
----------------
jdd wrote:
> rriddle wrote:
> > Please only use namespaces in source files for classes.
> Doesn't work in this case. Since FieldInfo is in the mlir namespace (with the rest of the Test dialect), these two functions have to be also. They do not have to be exposed outside of this file, so they're not declared in the header.
You need to fully qualify the methods: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions

You should very very rarely (most cases are bugs in the compiler, e.g. some templates in GCC 5) need to actually use a namespace in the .cpp to define a previously declared method.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:27
+using namespace mlir::tblgen;
+using namespace llvm;
+
----------------
nit: Can you please avoid `using namespace llvm` here? 


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:50
+    SmallSet<Dialect, 4> dialects;
+    for (auto typeDef : defs)
+      dialects.insert(typeDef.getDialect());
----------------
nit: Add `&` unless it is trivially copyable, using the type instead of auto would it make it a bit clearer what is being iterated here. 


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:73
+/// parameters.
+template <void (*T)(const TypeParameter &, raw_ostream &), bool PrependComma>
+class TypeParamCommaFormatter : public detail::format_adapter {
----------------
I would prefer that these just be function_ref and boolean fields of the class instead.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:74
+template <void (*T)(const TypeParameter &, raw_ostream &), bool PrependComma>
+class TypeParamCommaFormatter : public detail::format_adapter {
+  ArrayRef<TypeParameter> params;
----------------
This class needs to be wrapped in an anonymous namespace.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:75
+class TypeParamCommaFormatter : public detail::format_adapter {
+  ArrayRef<TypeParameter> params;
+
----------------
nit: Prefer having the members at the end of the class definition, not first.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:97
+/// as a value in formatv.
+typedef TypeParamCommaFormatter<emitTypeNamePair, false> emitTypeNamePairs;
+/// Emit `, parameter1Type parameter1Name, parameter2Type parameter2Name, [...]`
----------------
Please do not use typedef, prefer `using Foo = blah;` directives instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86904