This is an archive of the discontinued LLVM Phabricator instance.

Merge kDynamicSize and kDynamicSentinel into one constant.
ClosedPublic

Authored by khasanovaa on Nov 18 2022, 3:02 AM.

Diff Detail

Event Timeline

khasanovaa created this revision.Nov 18 2022, 3:02 AM
khasanovaa requested review of this revision.Nov 18 2022, 3:02 AM

Thanks a lot for your hard work on cleaning these unspeakable leaky magic constants over the past months ...

To really call this done we will want to make ShapedType::kDynamic private and force all users to go through safe APIs but I know it is still a lot of work left and it should not be your responsibility.

I'd suggest putting a PSA up along the lines that significant cleanups have landed and the goal is to make ShapedType::kDynamic private.
Then we can iterate over time and send small NFC commits cleaning up pieces incrementally.

What makes this easily parallelizable is we can just make the field private locally, fix some compiler errors, make it public again, test-land-rinse-repeat until exhaustion.

mlir/include/mlir/IR/BuiltinTypes.td
586–587

Drop this comment ?

mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
1889

something looks fishy here.

1895

something looks fishy here.

mlir/lib/Conversion/TosaToTensor/TosaToTensor.cpp
41

something looks fishy here.

mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
179

something looks fishy here.

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
704

Something looks fishy here ..

747

Something looks fishy here ..

Thank you @khasanovaa

mlir/include/mlir/IR/BuiltinTypes.td
586–587

I would even drop this function completely.

mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
1889
1895
mlir/lib/Conversion/TosaToTensor/TosaToTensor.cpp
41

That's actually the only case where we have -1 in MLIR Core: when kDynamic is stored in an attribute in TOSA. The correct way of fixing it is to have a custom printer/parser for the tosa::SliceOp::size attribute and store kDynamic in it. Otherwise, printing int_min in IR is quite ugly. That would be smth for TOSA team to figure out. @sjarus

mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
179

getDynamicStrideOrOffset is not needed anymore

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
704

I think it's fine, but i would use !ShapedType::isDynamic(dim) here instead

747

same as above

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
704

You're right I overindexed on seeing ShapedType::kDynamic and < n.value() in the same line while glancing through.
Indeed, please ignore.

747

You're right I overindexed on seeing ShapedType::kDynamic and < n.value() in the same line while glancing through.
Indeed, please ignore.

Resolving comments

Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 10:02 AM
Herald added a subscriber: jsetoain. · View Herald Transcript
pifon2a accepted this revision.Nov 18 2022, 10:09 AM

LGTM for sparse part since it is purely mechanical

Resolving merge conflicts

pifon2a accepted this revision.Nov 21 2022, 3:16 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 21 2022, 5:02 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.