- 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 :)
Details
- Reviewers
rriddle - Commits
- rGfa26a37d3699: [mlir] Add shaped container component type interface
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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() |
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. |
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(); }); ? |
Is there a place that we can link to for ShapedType?