We can propagate the shape from tosa.cond_if operands into the true/false
regions then through the connected blocks. Then, using the tosa.yield ops
we can determine what all possible return types are.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
19 | /// ? (for doxygen/documentation comments) | |
28 | How would you feel about us using standard lattice terminology here? E.g., meet, join, bottom (well could also use least) | |
50 | Is context here for future? | |
55 | Elide trivial braces ? (I know not your favorite style convention :)). You could consider a return with ternary operator too | |
73 | Passing nullptr as context here feels weird - I would have expected both lhs & rhs to be from same context (I mean they are context adjacent but you know what I mean) and then passing either of them along. | |
98 | [up to you] for range with llvm::zip ? (then you could do std::tie(lhsSize, rhsSize, resultSize) = inside and avoid indexing directly) | |
mlir/lib/Dialect/Tosa/IR/TosaOps.cpp | ||
1195 | This is where meet would have read easier for me :) [it only sometimes expand, and expand makes me think of something else] | |
mlir/lib/Dialect/Tosa/Transforms/TosaInferShapes.cpp | ||
64–69 | Could this be more of a action? (don't know how to phrase it, this just feels like a noun and makes me go "propagate what? and where is this thing passed in that needs to be propagated?") | |
132 | Is this what this pass does? |
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
19 | Added in additional slash. Not entirely familiar with doxygen documentation comments so if this is off update me. | |
28 | Changed to meet. I wasn't familiar with lattice terminology so I took a stab at expand. | |
50 | Removed. The original npcomp implementation retained the context. Not sure why. | |
98 | In this case, because we grab a reference to the result type I think we are stuck with direct indexing. Whenever I see an alternative it feels more confusing than adding the few extra lines. | |
mlir/lib/Dialect/Tosa/Transforms/TosaInferShapes.cpp | ||
64–69 | Rephrased to sound more like an action. | |
132 | Updated comment. Must have been leftover from the initial commit copypasta |
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
28 | NP, it isn't necessarily the most intuitive and so you could have made a judgement call for more readable :) (I always forget which one is which and also one can set it up in different directions - I mostly try and bring it back to And and Or [given the symbol reuse] and ask myself which one is more of an And vs more of an Or, but that is subjective) | |
113 | I think this join is the same as Shape_Join, except for the handling where the static dims don't match. There it is an error, here you are returning ?. These are used differently (there it is an equality constraint really) so makes sense. In ValueKnowledge there is no error state correct? So if I wanted to say shape x == shape y, then I couldn't here, it would have to be in shape function? | |
149 | And effectively needed to differentiate between scalar or not? | |
mlir/lib/Dialect/Tosa/Transforms/TosaInferShapes.cpp | ||
52 | Wouldn't it actually be an error if a static dim mismatches here? |
Updated to change hasSizes to hasRank, add an error state, and handle
the newly added error state.
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
113 | I changed to include an error state. I think the premise was to just continue with whatever we can but it feels reasonable to restrict down as best as possible. | |
149 | Less differentiate between scalar or not, with unranked and ranked. I changed to hasRank to clarify. | |
mlir/lib/Dialect/Tosa/Transforms/TosaInferShapes.cpp | ||
52 | Yup you are right. I added a check for error state and continued in cases where it can't propagate safely. |
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
84–87 | Should swap or merge the checks? If lhs or rhs is null, we will crash in the first if-statement right? | |
92–94 | style nit: drop the braces | |
113 | IIRC, we prefer for (auto i : llvm::seq<unsigned>(0, result.sizes.size())) style. | |
139–142 | ditto | |
mlir/lib/Dialect/Tosa/IR/TosaOps.cpp | ||
1170–1176 | llvm style nit: Use braces on the outer block because there are more than two levels of nesting. for ... { for ... if ... } |
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
84–87 | Technically its safe because ValueKnowledge is *not* part of the mlir type system. Its bool check is whether the value type is an error, not that it exists. Updated anyway. |
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h | ||
---|---|---|
136 | @rsuderman duplicate check if (!lhs || !rhs || lhs.dtype != rhs.dtype) Reported on https://pvs-studio.com/en/blog/posts/cpp/1003/ (N2) |
/// ? (for doxygen/documentation comments)