Page MenuHomePhabricator

[mlir] Add shaped container component type interface
ClosedPublic

Authored by jpienaar on Jan 8 2020, 7:57 PM.

Details

Summary
  • Add shaped container type interface which allows infering the shape, element type and attribute of shaped container type separately. Show usage by way of tensor type inference trait which combines the shape & element type in infering a tensor type;
    • All components need not be specified;
    • Attribute is added to allow for layout attribute that was previously discussed;
  • Expand the test driver to make it easier to test new creation instances (adding new operands or ops with attributes or regions would trigger build functions/type inference methods);
    • The verification part will be moved out of the test and to verify method instead of ops implementing the type inference interface in a follow up;
  • Add MLIRContext as arg to possible to create type for ops without arguments, region or location;
  • Also move out the section in OpDefinitions doc to separate ShapeInference doc where the shape function requirements can be captured;
    • Part of this would move to the shape dialect and/or shape dialect ops be included as subsection of this doc;
  • Update ODS's variable usage to match camelBack format for builder, state and arg variables;
    • I could have split this out, but I had to make some changes around these and the inconsistency bugged me :)

Diff Detail

Event Timeline

jpienaar created this revision.Jan 8 2020, 7:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2020, 7:57 PM

Unit tests: pass. 61321 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Just looked at the doc for now, will look at the rest later.

mlir/docs/ShapeInference.md
5

Is there a place that we can link to for ShapedType?

7

nit: expand ops to operations

8

op -> operation

9

nit: Can you reword to avoid starting with 'And'.

14

Is there a link for InferShapedTypeOpInterface? Maybe we should start generating/improve the docs for interfaces.

17

Can you reword? This sentence sounds a bit weird. The second part doesn't seem to flow from the first.

27

nit: Constraints

29

Is sign a know thing we can link to?

31

nit: Could we move the 'For example...' parts to a sub-bullet?

36

Should this be a NOTE:?

55

But seems like a weird start to a new section.

rriddle added inline comments.Jan 9 2020, 10:50 AM
mlir/include/mlir/Analysis/InferTypeOpInterface.h
19–20

Can you remove Types.h now? Some of the others could also be trimmed a bit as well.

34

-> /

70

nit: Can you pack this bool with either the type or the attribute?

83

Why not function_ref here? Also, is this function intended to be user facing? i.e. should other users just call this function?

mlir/lib/Analysis/InferTypeOpInterface.cpp
14–15

Trim these headers?

mlir/test/lib/TestDialect/TestDialect.cpp
318

nit: operands.getTypes()

mlir/test/lib/TestDialect/TestPatterns.cpp
74

Can you use an array or inline these values instead?

93

Drop the getOperations()

rriddle requested changes to this revision.Jan 9 2020, 1:27 PM
This revision now requires changes to proceed.Jan 9 2020, 1:27 PM
jpienaar updated this revision to Diff 237507.Jan 11 2020, 9:07 AM
jpienaar marked 21 inline comments as done.

Updated docs & switched to function_ref.

Unit tests: pass. 61655 tests passed, 0 failed and 777 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Apparently I forgot to hit submit (seeing unsubmitted comments below).

mlir/docs/ShapeInference.md
29

Not really, I'm referring to basic math. I'll spell it out a bit more.

mlir/include/mlir/Analysis/InferTypeOpInterface.h
70

Maybe, but it doesn't belong there logically: it matches dims and not the others. I'd prefer not pack it until needed (e.g., we could likely change how dims are stored here and then it could change the packing).

83

Made function_ref and moved to namespace detail as I expect this to be called via inferReturnTypes, and this is just to extract common function out to reduce size of InferTensorType<...> below.

rriddle accepted this revision.Jan 14 2020, 8:31 PM
rriddle added inline comments.
mlir/include/mlir/Analysis/InferTypeOpInterface.h
19–20

I would think that OpDefinition gives you the rest of these headers already.

mlir/test/lib/TestDialect/TestOps.td
406

Is this wrapped at 80 characters?

mlir/test/lib/TestDialect/TestPatterns.cpp
109

can you do:

.walk[this](InferTypeOpInterface retTypeFn) {

Operation *op = retTypeFn.getOperation();

});

?

This revision is now accepted and ready to land.Jan 14 2020, 8:31 PM
rriddle added inline comments.Jan 14 2020, 8:33 PM
mlir/include/mlir/Analysis/InferTypeOpInterface.h
31

I don't understand what the use case for the attribute is.

47

Seems like there is no constructor for the unranked case that can initialize the element type and attribute.

This revision was automatically updated to reflect the committed changes.