This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add native Bytecode support for properties
ClosedPublic

Authored by mehdi_amini on May 22 2023, 1:34 AM.

Details

Summary

This is adding a new interface (BytecodeOpInterface) to allow operations to
opt-in skipping conversion to attribute and serializing properties to native
bytecode.

The scheme relies on a new section where properties are stored in sequence

{ size, serialize_properties }, ...

The operations are storing the index of a properties, a table of offset is
built when loading the properties section the first time.

This is a re-commit of 837d1ce0dc which conflicted with another patch upgrading
the bytecode and the collision wasn't properly resolved before.

Diff Detail

Event Timeline

mehdi_amini created this revision.May 22 2023, 1:34 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added 1 blocking reviewer(s): jpienaar. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
mehdi_amini requested review of this revision.May 22 2023, 1:34 AM
mehdi_amini edited the summary of this revision. (Show Details)

Finish the implementation.

mehdi_amini edited the summary of this revision. (Show Details)

Use an index for encoding instead of an offset

Was this ready for review? Sorry if not (I still see a few TODOs in there). Looking pretty good though,

mlir/include/mlir/Bytecode/BytecodeOpInterface.h
15

Same below.

mlir/include/mlir/Bytecode/BytecodeOpInterface.td
30

Can you fill these out?

32–33
mlir/include/mlir/Bytecode/Encoding.h
94

Getting really close to using the full byte

mlir/lib/Bytecode/BytecodeOpInterface.cpp
2

Copy paste issue here.

mlir/lib/Bytecode/Reader/BytecodeReader.cpp
82–84

Why not just return version < 4?

974
1034–1038

?

1041
1044–1046
mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
417–418

Can you use optional instead here? -1 doesn't feel like a clean error condition here.

422–423

How does the reader handle when an operation is unregistered when written, but registered when read?

462–464
466
486–487

What happens for HUGE properties? Or should we not worry about those here?

842–844

How is this possible when we already checked the bytecode version in the if condition above?

mlir/lib/Bytecode/Writer/IRNumbering.cpp
297

Might want to add a note that the op is required to implement this interface, given that it's not obvious from just this context.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
1330

Fill this out?

mehdi_amini marked 15 inline comments as done.

Address comments

Was this ready for review? Sorry if not (I still see a few TODOs in there). Looking pretty good though,

Fixed the TODOs now, should be ready. The main design question I had was about the fact that we required every dialect to include the bytecodeopinterface explicitly, which is a bit of boilerplate...

mlir/include/mlir/Bytecode/Encoding.h
94

Next flag will indicate "two bytes" :)
Or we'll move to a varint?

mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
422–423

It won't indeed, I'm not sure how to do this actually?

486–487

What kind? On the operation they are size-limited by construction (there is a static_assert(sizeof(Properties) < ...) , for the referenced data it is up to the user, but IMO they should use resources for storage of huge data.

rriddle added inline comments.May 25 2023, 12:12 AM
mlir/include/mlir/Bytecode/Encoding.h
94

Varint is hard because we have to be able to patch it (and keep size), though maybe we could work around that by having a sentinel bit? Haven't thought about it much, as I didn't expect us to get close this fast...

mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
422–423

Only thing I can think of is to encode that it's using the unregistered form, and then extract properties from the attribute dictionary on read.

486–487

I forgot that we had guidance on size, what you have LGTM.

mehdi_amini added inline comments.May 25 2023, 12:24 AM
mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
422–423

Right but where to store this encoding? Seems like yet another bits on the operation flags?

rriddle added inline comments.May 25 2023, 12:31 AM
mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
422–423

I'd assume we could store it as extra information with the operation name? Given that it's the same info for every instance of this operation.

Record in the operation name whether the op was registered at the time of bytecode emission

rriddle accepted this revision.May 25 2023, 10:18 AM
rriddle added inline comments.
mlir/include/mlir/Bytecode/BytecodeOpInterface.h
2

Copy paste issue here

mlir/lib/Bytecode/Reader/BytecodeReader.cpp
1682

This is uninitialized on version < 4, can we initialize it with something?

mlir/lib/Bytecode/Writer/IRNumbering.cpp
303–304
mehdi_amini added inline comments.May 25 2023, 12:59 PM
mlir/lib/Bytecode/Reader/BytecodeReader.cpp
1682

I will make it an optional

This revision was not accepted when it landed; it landed in state Needs Review.May 25 2023, 3:16 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini marked 8 inline comments as done.
burmako added inline comments.
mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
636

Hi Mehdi! I noticed that this call (and its implementation) are not guarded by if (config.bytecodeVersion < 4). This seems to be breaking forward compatibility, which we found in StableHLO compatibility tests (unfortunately, we haven't yet had the time to write upstream compatibility tests, but that's planned).

~/burmako/v4.mlirbc:0:0: error: invalid section ID: 8
~/burmako/v4.mlirbc:0:0: note: in bytecode version 0 produced by: StableHLO_v0.9.0

Is this an intended breakage, or maybe we can keep forward compatibility for now?

burmako added inline comments.May 25 2023, 8:15 PM
mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
636

The fix appears to be easy. I've just proposed a patch: https://reviews.llvm.org/D151531.

mehdi_amini added inline comments.May 25 2023, 8:28 PM
mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
636

Ooops, sorry for the breakage, and thanks for catching it! :)
(and fixing it)

mehdi_amini reopened this revision.May 25 2023, 9:08 PM
mehdi_amini edited the summary of this revision. (Show Details)

Update to use version 5 (fix collision with jpienaar@'s patch that upgrade to 4)

Fix "Fix non-const lvalue reference to type 'uint64_t' cannot bind to type 'size_t' " on some platforms

This revision was not accepted when it landed; it landed in state Needs Review.May 26 2023, 5:45 PM
This revision was automatically updated to reflect the committed changes.