This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Dialect type/attr bytecode read/write generator.
ClosedPublic

Authored by jpienaar on Feb 25 2023, 10:45 PM.

Details

Summary

Tool to help generatedialect bytecode Attribute & Type reader/writing.

It helps reduce boilerplate when writing dialect bytecode attribute and
type readers/writers. It is not an attempt at a generic spec mechanism
but rather practically focussing on boilerplate reduction while also
considering that it need not be only in memory format and make it
relatively easy to change.

Diff Detail

Event Timeline

jpienaar created this revision.Feb 25 2023, 10:45 PM
jpienaar requested review of this revision.Feb 25 2023, 10:45 PM
jpienaar updated this revision to Diff 502403.Mar 4 2023, 10:02 PM

Updated post discussion to immediately use it to replace builtin dialect
reader/writer instead of starting more separate. Also bootstrapping was to
clarify unstable nature, but was called out to be too emphasized.

jpienaar retitled this revision from [mlir] Dialect type/attr bytecode bootstrapping tool. to [mlir] Dialect type/attr bytecode read/write generator..Mar 5 2023, 6:38 AM
jpienaar edited the summary of this revision. (Show Details)
jpienaar updated this revision to Diff 503180.Mar 7 2023, 4:22 PM

Fix format

Took an initial scan, brain too fried to look at tablegen backend code but will take a look tomorrow.

mlir/include/mlir/IR/BuiltinDialectBytecode.td
18–32 ↗(On Diff #503180)

Can we move these to BytecodeBase? These feel like generally useful constructs.

34–40 ↗(On Diff #503180)

I'd be fine with dropping these comments. I think the defs in most of these are easy enough to read as-is.

41 ↗(On Diff #503180)

Can you separate the differen attrs/types with comment blocks like our other code?

//===----------------------------------------------------------------------===//
// ArrayAttr
//===----------------------------------------------------------------------===//

I think it'd also help for reading the ones that need multiple tablegen defs.

65–66 ↗(On Diff #503180)
163–164 ↗(On Diff #503180)

Can you just use a base class for this instead of a multi-class? I was finding this a little hard to read when scanning (same for the others below)

mlir/include/mlir/IR/BytecodeBase.td
16 ↗(On Diff #503180)

Can you wrap this at 80-characters?

18–20 ↗(On Diff #503180)

Should we have more explicit names for the variables? That would match our other tablegen code more, but we are also likely to need much fewer of these than in other cases.

53–64 ↗(On Diff #503180)

Can you sprinkled a few comments on the top-level classes and defs in this file? Would help to understand what they are and how/when they get used. E.g we don't have any docs on DialectAttribute(s) and friends right now.

mlir/lib/IR/BuiltinDialectBytecode.cpp
60–72 ↗(On Diff #503180)

This one I'd likely expect to be provided directly by the infra, the others likely not.

98–106 ↗(On Diff #503180)

This is also one we'd need to shift to a more common place, right?

jpienaar updated this revision to Diff 505506.Mar 15 2023, 8:39 AM
jpienaar marked 7 inline comments as done.

Update to move common helpers to base & document defs/classes more.

mlir/include/mlir/IR/BuiltinDialectBytecode.td
163–164 ↗(On Diff #503180)

I'll just make this inline, make it basic :)

mlir/include/mlir/IR/BytecodeBase.td
18–20 ↗(On Diff #503180)

This is mostly only used for generic cases (e.g., reuse a string template with different variables), else one can use explicit names.

Or do you mean something like {reader} ?

It's looking good, would love a bit more documentation on the tablegen pieces. Took me a bit to remember the APIs and conventions there.

mlir/include/mlir/IR/BytecodeBase.td
18–20 ↗(On Diff #503180)

I was thinking of the $_reader convention we have in other places. Not a strong preference, but it wasn't clear how many of these we would need to define in practice (if often, having more explicit names feels a bit nicer).

mlir/tools/mlir-tblgen/BytecodeDialectGen.cpp
19 ↗(On Diff #505506)

Do we need the -rw at the end?

26 ↗(On Diff #505506)

-> / in this file?

Reminds me that the tablegen style is still a little all over the place

61 ↗(On Diff #505506)

static? Also can you document this?

70–72 ↗(On Diff #505506)
97 ↗(On Diff #505506)
141–142 ↗(On Diff #505506)

Can you document this logic a bit? It was a little hard to follow on first read.

193–196 ↗(On Diff #505506)

Can you split this into a helper? Would help with the scoping, and shrink the current function a bit

368–370 ↗(On Diff #505506)

Can you document this? Also wrap in anonymous namespace?

jpienaar updated this revision to Diff 512337.Apr 10 2023, 10:18 PM
jpienaar marked 7 inline comments as done.

Addressing most comments

Need to extract helper still, but before I forget to show updated form.

jpienaar updated this revision to Diff 512338.Apr 10 2023, 10:20 PM

Forgot to update command line arg.

jpienaar updated this revision to Diff 515206.Apr 19 2023, 8:53 PM

Split out helper function and update flag name.

jpienaar updated this revision to Diff 515208.Apr 19 2023, 9:11 PM
jpienaar marked 4 inline comments as done.

Move common helpers to move common location.

mlir/lib/IR/BuiltinDialectBytecode.cpp
60–72 ↗(On Diff #503180)

Wasn't sure the best spots for both of these but constrained them to bytecode implementation header.

jpienaar edited reviewers, added: mehdi_amini; removed: nicolasvasilache.Apr 20 2023, 9:13 PM
rriddle accepted this revision.Apr 23 2023, 11:35 PM

Would love some more comments in the internals of the tablegen code, but overall LGTM.

mlir/tools/mlir-tblgen/BytecodeDialectGen.cpp
217–219 ↗(On Diff #515208)

Can you document this? The name of this function took me a second to understand.

257 ↗(On Diff #515208)

Can we resolve this?

291–292 ↗(On Diff #515208)

make_second_range?

458 ↗(On Diff #515208)

Can you tighten the anonymous namespace to just wrap the classes?

This revision is now accepted and ready to land.Apr 23 2023, 11:35 PM
jpienaar marked 4 inline comments as done.Apr 24 2023, 10:28 AM

Thanks, and I'll follow up improving the internal documentation.

This revision was automatically updated to reflect the committed changes.

Seems like we're missing a doc for this?

Seems like we're missing a doc for this?

Ping?

Seems like we're missing a doc for this?

Ping?

https://reviews.llvm.org/rGf007bcbc3c8f3bcecdf0b4c0106b384aa09f794b was meant to address this.