This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tosa] Tosa shape propagation for tosa.cond_if
ClosedPublic

Authored by rsuderman on Jul 13 2021, 2:32 PM.

Details

Summary

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.

Diff Detail

Event Timeline

rsuderman created this revision.Jul 13 2021, 2:32 PM
rsuderman requested review of this revision.Jul 13 2021, 2:32 PM
rsuderman updated this revision to Diff 358430.Jul 13 2021, 2:37 PM

Added missing test.

jpienaar added inline comments.Jul 13 2021, 11:39 PM
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h
18

/// ? (for doxygen/documentation comments)

27

How would you feel about us using standard lattice terminology here? E.g., meet, join, bottom (well could also use least)

49

Is context here for future?

54

Elide trivial braces ? (I know not your favorite style convention :)). You could consider a return with ternary operator too

72

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.

97

[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
935

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
62

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?")

129

Is this what this pass does?

rsuderman updated this revision to Diff 358735.Jul 14 2021, 1:46 PM

Addressed clang-tidy errors and most of jpienaar@ comments

rsuderman updated this revision to Diff 358736.Jul 14 2021, 1:54 PM
rsuderman marked 5 inline comments as done.

Rest of jpienaar comments.

rsuderman marked 3 inline comments as done.Jul 14 2021, 1:55 PM
rsuderman added inline comments.
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h
18

Added in additional slash. Not entirely familiar with doxygen documentation comments so if this is off update me.

27

Changed to meet. I wasn't familiar with lattice terminology so I took a stab at expand.

49

Removed. The original npcomp implementation retained the context. Not sure why.

97

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
62

Rephrased to sound more like an action.

129

Updated comment. Must have been leftover from the initial commit copypasta

rsuderman updated this revision to Diff 358782.Jul 14 2021, 4:45 PM
rsuderman marked 2 inline comments as done.

Moved header guard

jpienaar added inline comments.Jul 16 2021, 9:38 AM
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h
27

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?

rsuderman updated this revision to Diff 359892.Jul 19 2021, 1:24 PM

Updated to change hasSizes to hasRank, add an error state, and handle
the newly added error state.

rsuderman marked 3 inline comments as done.Jul 19 2021, 1:26 PM
rsuderman added inline comments.
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.

hanchung added inline comments.Jul 22 2021, 3:13 PM
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
910–916

llvm style nit: Use braces on the outer block because there are more than two levels of nesting.

for ... {
  for ...
    if ...
}

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

rsuderman marked 3 inline comments as done.

Updated for @hanchung comments.

rsuderman marked 5 inline comments as done.Jul 26 2021, 10:25 AM
rsuderman added inline comments.
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.

rsuderman updated this revision to Diff 361872.Jul 26 2021, 6:01 PM
rsuderman marked an inline comment as done.

Added missing include for llvm::seq

rsuderman edited reviewers, added: mehdi_amini; removed: hanchung, scotttodd.Jul 29 2021, 3:01 PM
jpienaar accepted this revision.Aug 3 2021, 4:02 PM

Looks good thanks!

This revision is now accepted and ready to land.Aug 3 2021, 4:02 PM
This revision was automatically updated to reflect the committed changes.
RKSimon added inline comments.
mlir/include/mlir/Dialect/Tosa/Utils/ShapeUtils.h
137

@rsuderman duplicate check

if (!lhs || !rhs || lhs.dtype != rhs.dtype)

Reported on https://pvs-studio.com/en/blog/posts/cpp/1003/ (N2)