Page MenuHomePhabricator

[mlir] Add classes to define new TypeIDs at runtime
ClosedPublic

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
151

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
145
159

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.Oct 28 2021, 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.Nov 25 2021, 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.Nov 25 2021, 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.

rriddle accepted this revision.Dec 1 2021, 5:40 PM

LGTM, nice.

mlir/include/mlir/Support/TypeID.h
161

I'm not sure we need the NonMovable in the name of the type. Something like SelfOwningTypeID or something that better convey the usage of this class seems better.

163

?

169

If you end up removing Owner from the name, this could also just be an implicit conversion to TypeID.

This revision is now accepted and ready to land.Dec 1 2021, 5:40 PM

Hi, I was wondering about the status of this patch. I'd be interested in using it in my project.

math-fehr updated this revision to Diff 402699.Jan 24 2022, 3:34 PM

Address comments, and rename class to SelfOwningTypeID

math-fehr marked 3 inline comments as done.Jan 24 2022, 3:37 PM

Sorry, I just applied the last changes, and this should be ready to be committed I believe!
@rriddle, could you commit it for me if this is good for you? I do not have commit rights.

Sorry, I just applied the last changes, and this should be ready to be committed I believe!
@rriddle, could you commit it for me if this is good for you? I do not have commit rights.

Yeah, I'll land tomorrow.

Sorry, I just applied the last changes, and this should be ready to be committed I believe!
@rriddle, could you commit it for me if this is good for you? I do not have commit rights.

Yeah, I'll land tomorrow.

Woah, missed this. Can you reupload with arc? Alternatively, I need an email to land with.

No problem!

Could you land it with mathieu.fehr@gmail.com ? My arc is currently broken :/

This revision was automatically updated to reflect the committed changes.