Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. | |
747 | You're right I overindexed on seeing ShapedType::kDynamic and < n.value() in the same line while glancing through. |
Drop this comment ?