Page MenuHomePhabricator

[mlir] Support for defining Types in tblgen
ClosedPublic

Authored by jdd on Aug 31 2020, 8:28 PM.

Details

Summary

Adds a TypeDef class to OpBase and backing generation code

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rriddle added inline comments.Thu, Oct 1, 8:03 PM
mlir/include/mlir/TableGen/TypeDef.h
133

Seems like TypeDef could just provide a range of it's parameters and callers of this could just you llvm::map_range instead.

mlir/test/lib/Dialect/Test/TestTypes.cpp
29

nit: Drop all of the mlir:: in this file, and throughout the revision.

102

The definitions of methods in header files should be fully qualified :: and not placed in a namespace in the .cpp.

mlir/tools/mlir-tblgen/OpDocGen.cpp
172

nit: Prefer early return instead.

175

nit: Unless the end loop iterator changes, cache it.

mlir/tools/mlir-tblgen/TypeDefGen.cpp
84

The name of this parameter doesn't seem to match what you are doing here.

86

Could you just format the parameters into a llvm::raw_string_ostream instead of constructing separate strings?

99

Same comment as above.

135

This looks like the indent is off.

187

Do we need to generate this method? Seems like the uses of it by the generator could just inline the string instead.

198

nit: Spell out auto here, also use a reference instead of by-value.

223

Seems like this would be something only generated in the source file and marked as static.

275

This looks over indented, should be 2 spaces not 4.

294

This looks misaligned.

312

nit: Spell out auto here and don't use a by-value iterator variable.

349

Should these be auto &instead?

374

Please remove all of the personal comments from the revision.

382

nit: Comments should be complete sentences. This applies throughout the revision.

421

nit: Same comment about auto here as above.

482

Stray comment?

506

Can we use a LogicalResult return instead?

514

Can we avoid generating print/parse on the types themselves? We should try to keep as much internal details hidden away as possible.

This revision now requires changes to proceed.Thu, Oct 1, 8:03 PM
jdd added a comment.EditedFri, Oct 2, 12:34 PM

As for declaring parse(...), print(...), and getMnemonic() in the header file: these are intended for cases where (for some reason) the dialect doesn't use the global parse/print dispatch method. The dialect could call each parse/print method in its Dialect::parseType/Dialect::printType methods, in concert with the getMnemonic(). I figure even if it's not often used, its just an extra 3 lines in each type declaration.

Do you agree or should I remove it?

mlir/include/mlir/IR/OpBase.td
2377

Ah. I thought proper punctuation was only necessary after full sentences.

jdd marked an inline comment as done.Fri, Oct 2, 7:19 PM
jdd added inline comments.
mlir/test/lib/Dialect/Test/TestTypes.cpp
64

Nice!

94

That's handy

102

I've removed two from the header

mlir/tools/mlir-tblgen/TypeDefGen.cpp
223

The dialect calls these from its parseType and printType functions so we must declare them here.

jdd marked 32 inline comments as done.Fri, Oct 2, 8:40 PM
jdd edited the summary of this revision. (Show Details)
jdd updated this revision to Diff 295953.Fri, Oct 2, 8:43 PM
jdd edited the summary of this revision. (Show Details)

Revisions base on River's comments

jdd marked 2 inline comments as done.Fri, Oct 2, 8:45 PM
jdd updated this revision to Diff 295955.Fri, Oct 2, 8:46 PM

Missed a comment

jdd updated this revision to Diff 296316.Mon, Oct 5, 3:30 PM

clang-format

Really great stuff, I'm glad you've been pushing this! Mostly nits left for the code, it's enough that we can just iterate further in tree. Can you document all of this cool new stuff? Either in docs/OpDefinitions or a new docs/TypeDefinitions?

mlir/include/mlir/TableGen/TypeDef.h
72

nit: Can we remove the llvm:: on these as well?

94

Is the llvm:: here necessary?

mlir/lib/TableGen/TypeDef.cpp
153

Please drop else after a return.

155

nit: Drop the != nullptr.

mlir/test/lib/Dialect/Test/TestTypeDefs.td
119

Please cache the end iterator of loops if the size doesn't change.

120

The formatting here seems off.

mlir/test/lib/Dialect/Test/TestTypes.cpp
37

nit: Drop trivial braces here. Also if you use braces on one if/else, please use it for all of them.

88

Please only use namespaces in source files for classes.

106

nit: Drop the newline here.

mlir/tools/mlir-tblgen/OpDocGen.cpp
243

nit: Drop trivial braces here.

mlir/tools/mlir-tblgen/TypeDefGen.cpp
58

Please use braces on every else if they are used on one.

64

nit: auto -> const TypeDef &

83

llvm::interleaveComma?

100

Same here?

310

Can you stream the tgfmt directly into os?

434

Can you stream it into os directly?

jdd marked 16 inline comments as done.Fri, Oct 9, 10:21 PM

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!

mlir/include/mlir/TableGen/TypeDef.h
72

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.

mlir/test/lib/Dialect/Test/TestTypes.cpp
88

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.

mlir/tools/mlir-tblgen/TypeDefGen.cpp
83

Ohhh. I'll do ya one better. Lemme know if the is too far deep into C++ trickery territory.

jdd updated this revision to Diff 297386.Fri, Oct 9, 10:22 PM
jdd marked an inline comment as done.
  • Changes based on review
  • Remove llvm:: from the cpp file
jdd marked an inline comment as done.Fri, Oct 9, 10:31 PM
jdd updated this revision to Diff 297438.Sat, Oct 10, 7:16 PM
  • Clang-tidy fix
rriddle accepted this revision.Tue, Oct 13, 11:45 AM
In 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.

mlir/include/mlir/TableGen/TypeDef.h
72

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

mlir/test/lib/Dialect/Test/TestTypes.cpp
88

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.

mlir/tools/mlir-tblgen/TypeDefGen.cpp
28

nit: Can you please avoid using namespace llvm here?

51

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.

74

I would prefer that these just be function_ref and boolean fields of the class instead.

75

This class needs to be wrapped in an anonymous namespace.

76

nit: Prefer having the members at the end of the class definition, not first.

98

Please do not use typedef, prefer using Foo = blah; directives instead.

This revision is now accepted and ready to land.Tue, Oct 13, 11:45 AM
jdd marked 8 inline comments as done.Tue, Oct 13, 2:10 PM
jdd added inline comments.
mlir/tools/mlir-tblgen/TypeDefGen.cpp
74

That kinda defeats the syntactic niceness. Will convert this to normal class w/ enum to determine the format. This will make the calling syntax longer but preserve the efficiency.

jdd updated this revision to Diff 297957.Tue, Oct 13, 2:15 PM
jdd marked an inline comment as done.
  • Another round of changes based on feedback.
jdd updated this revision to Diff 297968.Tue, Oct 13, 3:00 PM
  • Rebasing w/ conflicts
  • New branch, applying everything at once
  • Clang-(format|tidy) fixes
  • One more clang-tidy fix
  • Changes based on Chris' comments
  • Ending a sentence comment w/ a period
  • Revisions base on River's comments
  • Clang-tidy and some missed comments
  • Missed a comment
  • clang-format
  • Changes based on review
  • Remove llvm:: from the cpp file
  • Clang-tidy fix
  • Another round of changes based on feedback.
This revision was automatically updated to reflect the committed changes.
In 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.

When you get to landing the doc, please update the newsletter on Discourse :)
This is a really cool feature.