This is an archive of the discontinued LLVM Phabricator instance.

Relax the constraint for MemRef type to allow any non-builtin type as element type.
Needs RevisionPublic

Authored by chhe on Oct 1 2020, 4:22 PM.

Details

Reviewers
rriddle
ftynse
Summary

Relax the constraint for MemRef type to allow any non-builtin type as element type.

Diff Detail

Event Timeline

chhe created this revision.Oct 1 2020, 4:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
chhe requested review of this revision.Oct 1 2020, 4:22 PM
rriddle added inline comments.Oct 1 2020, 4:32 PM
mlir/include/mlir/IR/StandardTypes.h
462

Can you move this to the .cpp file instead so that we don't need another include here?

chhe updated this revision to Diff 295695.Oct 1 2020, 4:56 PM

Move implementation to .cpp file.

The revision says that it allows "QuantType" but the implementation is actually making it accept any non-builtin type?

chhe retitled this revision from Relax the constraint for MemRef type to allow QuantType as element type. to Relax the constraint for MemRef type to allow any non-builtin type as element type..Oct 1 2020, 10:40 PM
chhe edited the summary of this revision. (Show Details)
chhe added a comment.EditedOct 1 2020, 10:43 PM

The revision says that it allows "QuantType" but the implementation is actually making it accept any non-builtin type?

Revision updated.

Quant element type for MemRef is the feature request. However, I think it may be inappropriate to explicitly only allow QuantType(it needs to add QuantType dependency to StandardType). So I just sync the constraint with TensorType.

ftynse requested changes to this revision.Oct 2 2020, 12:31 AM
ftynse added a subscriber: ftynse.

Similar changes have been proposed repeatedly. I am yet to see an RFC (which we normally require for standard/built-in changes) that clarifies the semantics of such memrefs. In particular, I am interested in the allocation/deallocation of such memrefs and any operations that require knowing the size of the elemental type. For example, how does one allocate memref<42 x opaque>? In general, there should be a clear specification of what dialects should check for in order for the type to be "valid" in a memref.

This revision now requires changes to proceed.Oct 2 2020, 12:31 AM
chhe removed a reviewer: grosul1.Oct 2 2020, 3:47 PM
chhe updated this revision to Diff 295929.Oct 2 2020, 4:22 PM

Add SizedTypeInterface and implement this interface for QuantType.

chhe updated this revision to Diff 295930.Oct 2 2020, 4:25 PM

Fix interface function comment.

rriddle requested changes to this revision.Oct 2 2020, 4:27 PM
rriddle added inline comments.
mlir/include/mlir/Interfaces/SizedTypeInterface.td
20 ↗(On Diff #295929)

I would prefer to see more discussion around such an interface first. This interface as proposed seems very simplistic, and not something that would be able to support LLVM for example.

27 ↗(On Diff #295929)

Along the same lines as the above, I'm not sure bytes are a good metric to use when considering size given that bytes can vary between platforms. I would prefer to have an explicit proposal for this interface that has its own motivations other than for supporting non-builtin types in Memref.

This revision now requires changes to proceed.Oct 2 2020, 4:27 PM
Jing added a subscriber: Jing.Oct 5 2020, 1:02 PM