Page MenuHomePhabricator

[mlir] Refactor OpInterface internals to be faster and factor out common bits.
ClosedPublic

Authored by rriddle on Mon, Jun 15, 2:16 PM.

Details

Summary

This revision adds a new support header, InterfaceSupport, to contain various generic bits of functionality for implementing "Interfaces". Interfaces embody a mechanism for attaching concept-based polymorphism to a type system. With this refactoring a new InterfaceMap type is added to allow for efficient interface lookups without going through an indirect call. The map is constructed such that only one instance exists for a given set of interface types. This should provide a decent performance speedup without changing the size of AbstractOperation.

In a future revision, this functionality will also be used to bring Interface like functionality to Attributes and Types.

Diff Detail

Event Timeline

rriddle created this revision.Mon, Jun 15, 2:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 15, 2:16 PM
mehdi_amini added inline comments.Fri, Jun 19, 12:41 AM
mlir/include/mlir/IR/OpDefinition.h
1373

The two "using" with the same name back-to-back are confusing to me: I'm not sure what this is doing?

mlir/include/mlir/Support/InterfaceSupport.h
49

I find these template parameters confusing (except the Traits, maybe because of the example).

For example: the description starts with This class represents the base of an interface but the last parameter is BaseTrait: The base type for the interface trait.?

Is the plan to use this class for Dialect interface?
I think it'd be great to expand on this a bit: This class represents the base of an interface: either an OpInterface or a DialectInterface

109

Class doc? Something like:

/// Map an InterfaceType to a particular implementation of the concept.
/// This class is a thin-wrapper to a a singleton-instance of the map for 
/// a given (ordered) list of interfaces.
112

It'd be nice to expand here:

  1. why do we have this possibility of having non-interface types (because we just forward a mix of traits and interfaces from the OpDefinition?)
  2. mention that the types that aren't interface are not added to the map
135

Is this general enough for STLExtras?

163

Won't this cause a large amount of dynamic initialization here?
Would having something like https://github.com/serge-sans-paille/frozen in-tree help?
At the same time we store pointers, I don't know if in the common build mode we could have this in a rodata section?

rriddle updated this revision to Diff 272195.Fri, Jun 19, 3:10 PM
rriddle marked 9 inline comments as done.

Address review.

rriddle added inline comments.Fri, Jun 19, 3:12 PM
mlir/include/mlir/IR/OpDefinition.h
1373

The first one creates an alias, the second inherits the constructor.

mlir/include/mlir/Support/InterfaceSupport.h
49

Cleaned up the doc a bit.

Is the plan to use this class for Dialect interface?

Not at the moment given that dialects don't need/use traits or concept based polymorphism and the usage model is much different. I do have plans to experiment with allowing dialects to provide a fallback for operation interfaces, which could end up aligning the implementations.

135

It could likely be generalized, but not in this revision.

163

Yes, what we really want is a frozen map given that we can generate it effectively in a const way. Switched to using unique_ptr for the map storage.

mehdi_amini accepted this revision.Fri, Jun 19, 8:27 PM
mehdi_amini added inline comments.
mlir/include/mlir/Support/InterfaceSupport.h
39

ValueT is adding some confusion to me, where is the terminology coming from?

I think it confuses me because Value is also a class in MLIR and use this pervasively. Here it is currently instantiated with an Operation* : having pointer described as a "value type" is also contributing to the confusion probably.

I suspect also that in my head I never consider that the operation interface "operates on an Operation" and so the sentence wasn't immediately clear to me.

What about:

ValueT: The opaque type the derived interface operates on. For example `Operation*` for Operation interfaces, or `Attribute` for AttributeInterface.
49

Is the plan to use this class for Dialect interface?

I actually reviewed this revision before reading that this was about refactoring in prevision of Attributes and Types...
Much more clear now!

57

This is a non-templated class -> it is instantiated with Op<ConcreteType>.

What about:

* BaseType: A desired base type for the interface. This is a class that provides
                    that provides specific functionality for the `ValueT` value. For 
                    instance the specific Op that will wrap the Operation* for an
                    OpInterface.
This revision is now accepted and ready to land.Fri, Jun 19, 8:27 PM

OOC did you try creating a benchmark to see performance impact/improvement from more efficient lookup?

jpienaar accepted this revision.Mon, Jun 22, 10:26 AM

Looks good further (just style nits)

mlir/include/mlir/IR/OpDefinition.h
1382–1383

Above ` was used, here ' , is this distinction important?

mlir/include/mlir/Support/InterfaceSupport.h
10

Incomplete sentence?

42

nit: s/as the/as the/

46

Nit:

```c++ ?
186

Could we combine this public section with the top one?

rriddle updated this revision to Diff 272528.Mon, Jun 22, 1:01 PM
rriddle marked 9 inline comments as done.

Resolve comments

mlir/include/mlir/IR/OpDefinition.h
1382–1383

I like being inconsistent.

This revision was automatically updated to reflect the committed changes.