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

[John Demme via Phabricator via llvm-commits]

jdd marked 16 inline comments as done.
jdd added a comment.

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.

Meanwhile, I've addressed all your comments. No matter how carefully I examine my code, I just keep missing your nits. I gotta learn to write code in the LLVM style. It's pretty much the complete opposite of how I'm used to writing code!



================
Comment at: mlir/include/mlir/TableGen/TypeDef.h:71
+  // supposed to auto-generate them.
+  llvm::Optional<StringRef> getMnemonic() const;
+
----------------
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.


================
Comment at: mlir/test/lib/Dialect/Test/TestTypes.cpp:87
+
+namespace mlir {
+
----------------
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.


================
Comment at: mlir/tools/mlir-tblgen/TypeDefGen.cpp:82
+    retStream << ", ";
+  for (auto it = parameters.begin(), e = parameters.end(); it < e; ++it) {
+    retStream << it->getCppType() << " " << it->getName();
----------------
rriddle wrote:
> llvm::interleaveComma?
Ohhh. I'll do ya one better. Lemme know if the is too far deep into C++ trickery territory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86904