This is an archive of the discontinued LLVM Phabricator instance.

[mlir][IR] Add RankedShapedType interface
Needs ReviewPublic

Authored by springerm on Apr 16 2023, 12:25 AM.

Details

Summary

RankedShapedType is a sub-interface of ShapedType and implemented by RankedTensorType, MemRefType and VectorType. Interface methods and helper methods that only make sense for ranked types are moved to RankedShapedType. ShapedType now only has interface methods and helper functions that are guaranteed to be supported by the type (no more failed runtime assertions).

RFC: https://discourse.llvm.org/t/rfc-shapedtype-type-interface-cleanup/69998

Depends On: D148488

Diff Detail

Event Timeline

springerm created this revision.Apr 16 2023, 12:25 AM
springerm requested review of this revision.Apr 16 2023, 12:25 AM
springerm edited the summary of this revision. (Show Details)Apr 16 2023, 12:26 AM

The pre-merge checks indicates some legit issues in Flang I believe.

springerm updated this revision to Diff 514068.Apr 16 2023, 5:38 PM
springerm edited the summary of this revision. (Show Details)

fix flang failures

springerm updated this revision to Diff 514070.Apr 16 2023, 5:42 PM

fix flang failures

Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 5:42 PM
rkayaith added inline comments.
mlir/include/mlir/IR/BuiltinTypeInterfaces.td
86–97

hasRank doesn't make sense as a method here, since it should always be true

springerm edited the summary of this revision. (Show Details)Apr 16 2023, 7:57 PM
springerm marked an inline comment as done.

Overall this LGTM, but I think we should wait for the RFC to get enough attention first (and other people to have a more careful look here as well maybe)

mlir/include/mlir/IR/BuiltinTypeInterfaces.td
101

Nit maybe, but you wrote "or not statically known" like if there is a difference from "dynamic", do you intend it? If so what is the difference?

112–113

The comment needs an update

134

Can you update the comment to indicate the behavior in case of dynamic shapes?

137

This method looks dangerous to me because of this caveat right now :(

It also does not seem easy to use before of the caveat mentioned below ("if its elemental type does not have a known bit width" without more precision about what element types are handled).

springerm marked 3 inline comments as done.Apr 16 2023, 10:52 PM
springerm added inline comments.
mlir/include/mlir/IR/BuiltinTypeInterfaces.td
137

Should this be an interface method? Tensor types don't materialize in memory, so there's no widening. I think this only applies to MemRefType and VectorType.

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

address comments

rriddle added inline comments.Apr 16 2023, 10:53 PM
mlir/include/mlir/IR/BuiltinAttributes.td
249–251
390–392

Please reflow this as suggested above.

844–847

Same here and elsewhere.

mlir/include/mlir/IR/BuiltinTypeInterfaces.td
137

Yes, IMO ideally this should be killed off. It's a relic from the pre-interface days.

mlir/include/mlir/IR/BuiltinTypes.td
379–381
mlir/lib/AsmParser/AttributeParser.cpp
1053–1060

I don't think there is much value is having multiple casts here

springerm marked 5 inline comments as done.Apr 17 2023, 1:05 AM
springerm updated this revision to Diff 514129.Apr 17 2023, 1:05 AM

rebase and address comments

springerm marked an inline comment as done.Apr 17 2023, 1:15 AM
springerm updated this revision to Diff 514132.Apr 17 2023, 1:16 AM

address comments

mehdi_amini added inline comments.Apr 17 2023, 2:51 AM
mlir/include/mlir/IR/BuiltinTypeInterfaces.td
137

It probably should also be its own interface or somehow be part of the DL dialect to be properly supported?

The changes to sparse module are all rather mechanical, so LGTM for that, assuming other reviewers approve the overall change

Could you please expose the python bindings for the RankedShapedType interface. My downstream project uses ShapedType from python quite a bit and moving all its methods without exposing RankedShapedType will break a lot of code.

springerm updated this revision to Diff 514506.Apr 17 2023, 8:03 PM

update CAPI and Python bindings

springerm updated this revision to Diff 514514.Apr 17 2023, 8:46 PM

clang-format

rkayaith added inline comments.Apr 17 2023, 9:33 PM
mlir/include/mlir/IR/BuiltinTypeInterfaces.td
154–157

would it make sense to return kDynamic instead of aborting?

mlir/lib/Bindings/Python/IRTypes.cpp
337–342 ↗(On Diff #514506)

The existing API seemed to be idiomatic for python, I'm not sure these needed to be removed from the ShapedType bindings (though better docstrings indicating they may fail would be great).

mlir/lib/CAPI/IR/BuiltinTypes.cpp
177

address comments

springerm marked 4 inline comments as done.Apr 17 2023, 10:57 PM
springerm added inline comments.
mlir/include/mlir/IR/BuiltinTypeInterfaces.td
154–157

I put this in a separate revision because the change is unrelated to this refactoring: D148605

mlir/include/mlir/IR/BuiltinTypes.td
379–381

I reformatted this so that it fits in two lines. Let me know if it should be formatted differently.

mlir/lib/Bindings/Python/IRTypes.cpp
337–342 ↗(On Diff #514506)

I added back the get_rank property.

I think there's no benefit in keeping the other ones on ShapedType because they would always fail on unranked types. Now they just fail with a different error (method does not exist vs. exception). Python is dynamically typed, so any method can be called on any object, we don't need them on ShapedType to make the type checker happy.

springerm retitled this revision from [mlir] Add RankedShapedType interface to [mlir][IR] Add RankedShapedType interface.
rkayaith added inline comments.Apr 18 2023, 8:48 AM
mlir/lib/Bindings/Python/IRTypes.cpp
337–342 ↗(On Diff #514506)

ah you're right, I forgot there's not direct TensorType equivalent in the bindings.

springerm marked 3 inline comments as done.

rebase

I'm going to break this up into smaller chunks to make it easier to review.