Page MenuHomePhabricator

[mlir] Add a generic DialectResourceBlobManager to simplify resource blob management
ClosedPublic

Authored by rriddle on Jul 18 2022, 8:59 AM.

Details

Summary

The DialectResourceBlobManager class provides functionality for managing resource blobs
in a generic, dialect-agnostic fashion. In addition to this class, a dialect interface and custom
resource handle are provided to simplify referencing and interacting with the manager. These
classes intend to simplify the work required for dialects that want to manage resource blobs
during compilation, such as for large elements attrs. The old manager for the resource example
in the test dialect has been updated to use this, which provides and cleaner and more consistent API.

This commit also adds new HeapAsmResourceBlob and ImmortalAsmResourceBlob to simplify
creating resource blobs in common scenarios.

Diff Detail

Event Timeline

rriddle created this revision.Jul 18 2022, 8:59 AM
rriddle requested review of this revision.Jul 18 2022, 8:59 AM
rriddle updated this revision to Diff 446078.Jul 20 2022, 1:48 AM

Nice! I just did a brief skim as I'm OOO this afternoon but priority tomorrow :)

mlir/include/mlir/IR/AsmState.h
86

Why is alignment needed for deleter?

95–98

I'm not a fan of unsigned as you know ;-)

186

/*dataIsMutable=*/true ?

So no option here for immutable data as there is no initializer?

205

Unmanged/Unowned ? The guaranteed to persist and immortal may make folks thing they do not need to worry about lifetimes, while this is opposite this class doesn't care ensure lifetimes.

210

It is unclear why this is always immutable, e.g., even if I don't own it and just want create a blob to handle memory I manage in a different way, I may be OK in mutating it. And this is probably also an invariant that has to be added to 205 if an external process is allowed to change this too. Say this was in a reference counted outer container of a framework, this is saying the framework is not allowed to release this memory or change the data while compilation is happening if I'm not mistaken (unless we completely treat the contents as opaque this could invalidate data dependent passes).

253

And up to implementation as to if owned or unowned?

Nice helpers/simple to use.

mlir/include/mlir/IR/DialectResourceBlobManager.h
132

Could we avoid shared_ptr? I'd expect the lifetime of the blob manager to be exceed that of the MLIRContext.

mlir/lib/IR/DialectResourceBlobManager.cpp
60

Could you add test that has multiple overlapping names with different data?

rriddle updated this revision to Diff 447533.Jul 25 2022, 6:25 PM
rriddle marked 7 inline comments as done.
rriddle added inline comments.Jul 25 2022, 6:26 PM
mlir/include/mlir/IR/AsmState.h
86

Some aligned allocators want the alignment used when the data was allocated (e.g. llvm::deallocate_buffer).

95–98

Switched to size_t, which is what the standard uses for alignment values.

186

Added a flag for mutability just in case a user wants to allocate (but not allow mutation).

210

I switched to using "Unmanaged" and added a flag for mutability. I think that is cleaner overall, and given that these are mostly just utility for construction I don't think it's a big deal to make mutability configurable.

253

Exactly, whatever created the blob already made those decisions.

mlir/include/mlir/IR/DialectResourceBlobManager.h
132

I wanted a nice default for the places that don't care about sharing and just want an easily configurable default (e.g. LSP server, etc.). If we don't use shared_ptr, we'd need an optional backing storage in each instance just in case the user doesn't want to explicitly provide one. Using shared_ptr felt fine here given that: there are relatively few lifetime changes (which would actually incur the cost of shared_ptr), dereferencing is the same as a normal pointer, etc.

mlir/lib/IR/DialectResourceBlobManager.cpp
60

The original resource patch actually tested for this, which should now be covered by this functionality (given that it replaced the adhoc stuff previously added to the test dialect): https://github.com/llvm/llvm-project/blob/78015047b22dd64f7c647ab7ce04d42e761e7b8f/mlir/unittests/Parser/ResourceTest.cpp#L29

jpienaar accepted this revision.Jul 26 2022, 6:56 AM

Looks good, thanks!

This revision is now accepted and ready to land.Jul 26 2022, 6:56 AM
rriddle updated this revision to Diff 448419.Jul 28 2022, 12:48 PM