Page MenuHomePhabricator

[mlir] Add classes to define new TypeIDs at runtime
Needs ReviewPublic

Authored by math-fehr on Jun 18 2021, 7:07 AM.

Details

Summary

TypeIDAllocator enables the allocation of new TypeIDs at runtime,
that are unique during the lifetime of the allocator.

NonMovableTypeIDOwner is a class used to define a new TypeID for each instance
of a class, using the instance address. This class cannot be copied or moved.

Diff Detail

Event Timeline

math-fehr created this revision.Jun 18 2021, 7:07 AM
math-fehr requested review of this revision.Jun 18 2021, 7:07 AM
math-fehr edited the summary of this revision. (Show Details)Jun 18 2021, 7:08 AM
math-fehr added a reviewer: mehdi_amini.
bondhugula added inline comments.
mlir/include/mlir/Support/TypeID.h
149

Nit: Since this is a TypeID allocator, you can name this allocate().

rriddle requested changes to this revision.Jun 21 2021, 11:47 AM
rriddle added inline comments.
mlir/include/mlir/IR/MLIRContext.h
147 ↗(On Diff #353000)

Why does this need to be in the context? Couldn't the TypeID allocator be owned by whatever is using it? (e.g. the extensible dialect)

mlir/include/mlir/Support/TypeID.h
143
157

Why not use a BumpPtrAllocator here instead? It doesn't look like you need the individual instances.

This revision now requires changes to proceed.Jun 21 2021, 11:47 AM
math-fehr updated this revision to Diff 353637.Jun 22 2021, 7:04 AM

Rename function, and use bump pointer allocator

math-fehr updated this revision to Diff 353640.Jun 22 2021, 7:13 AM

Fix spelling error

math-fehr accepted this revision.Jun 22 2021, 7:39 AM
math-fehr marked 3 inline comments as done.
This comment was removed by math-fehr.
mlir/include/mlir/IR/MLIRContext.h
147 ↗(On Diff #353000)

We could put a TypeID allocator in the extensible dialects for the operations and types TypeID.
However, we would still have a problem with the Dialects and Traits defined at runtime, since they need to be stored in the MLIRContext (I do not have a better solution for that currently).

There is however another solution we could consider, which is to create a class (let's call it DynamicObject) which hold a TypeID, as well as the storage if necessary. If we would make the DynamicOp/DynamicDialects/... all child classes of this DynamicObject, we wouldn't need any TypeIDAllocator, beside for the operations (since they are stored entirely in the AbstractOperation class). Also, alternatively, we could use the address of the DynamicTypeDefinition for the TypeID (which would give us for free a lookup for the type given the TypeID).

math-fehr resigned from this revision.Jun 23 2021, 7:37 AM

Sorry, I think I added myself as a reviewer by mistake.

@rriddle, a gentle ping on this.
What do you think would be the best course of action here?
As I said in a previous comment, I could either keep the TypeIDAllocator in the MLIRContext, or move it to the ExtensibleDialect and having the "dynamic" objects owning their TypeIDs.

Hey @rriddle, a gentle ping on this ^

rriddle added inline comments.Thu, Oct 28, 10:24 AM
mlir/include/mlir/IR/MLIRContext.h
147 ↗(On Diff #353000)

I really like the idea of a DynamicObject of some kind, which could internally use its own address for the TypeID? I think something like this could be placed directly within TypeID.h. The name would need to be different of course.

math-fehr updated this revision to Diff 389896.Thu, Nov 25, 6:59 PM

Add NonMovableTypeIDOwner, and remove TypeIDAllocator from MLIRContext

math-fehr retitled this revision from [mlir] Add TypeIDAllocator class to [mlir] Add classes to define new TypeIDs at runtime.Thu, Nov 25, 7:03 PM
math-fehr edited the summary of this revision. (Show Details)

Was this class what you add in mind? I can also define another class that owns its TypeID by having a pointer to some place in memory if this can be useful.