This is an archive of the discontinued LLVM Phabricator instance.

[tosa][mlir] Add dynamic width/height support for depthwise convolution in tosa-to-linalg
ClosedPublic

Authored by NatashaKnk on Mar 16 2022, 5:45 PM.

Details

Summary

In addition, fixed a small bug with padding incorrectly inferring output shape for dynaic inputs in convolution

Diff Detail

Event Timeline

NatashaKnk created this revision.Mar 16 2022, 5:45 PM
Herald added a project: Restricted Project. · View Herald Transcript
NatashaKnk requested review of this revision.Mar 16 2022, 5:45 PM
rsuderman added inline comments.Mar 21 2022, 11:25 AM
mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
51–54

You should be able to use ShapedType::kDynamicSize instead of -1. Makes things clearer what -1 means.

110

Rather than putting width / height int he loop, just copy the array then special case the width / height after. It limits the loop body but still handles the special cases.

jpienaar added inline comments.
mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
51–54

Or ShapedType::isDynamic(inputShape[i])

106

And here we will always have a rank?

111

Consider continuing if not dynamic to reduce the nesting here.

113

stride_y ?

Add small clarity fixes

NatashaKnk marked 3 inline comments as done.Mar 23 2022, 10:11 AM
NatashaKnk added inline comments.
mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
51–54

Is there a preferred approach between ShapedType::isDynamic(inputShape[i]) and something like inputTy.isDynamicDim(i)?

106

I'm assuming that running convolution on unranked input should error, correct? If that's the case, should I check for it in the outside function as a sanity check?

111

I'm guessing it won't be necessary if I follow Robs advice above and take the special cases out of the loop?

113

I'm not sure I'm understanding the question, but stride_y is how the tosa spec refers to the height parameter for stride.

jpienaar accepted this revision.Mar 24 2022, 11:17 AM

Nice, thanks

mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
106

You are correct in the verifier of conv we check if ranked.

113

Oh I just meant there is a typo sride instead of stride

137

dynDims[weightDim]?

183

kwarg style comments for 1 and 2 ? (that way we can clang-tidy check too)

This revision is now accepted and ready to land.Mar 24 2022, 11:17 AM
NatashaKnk updated this revision to Diff 420335.Apr 4 2022, 3:50 PM
NatashaKnk marked an inline comment as done.

Small additional cleanup

NatashaKnk updated this revision to Diff 421195.Apr 7 2022, 7:09 AM

Fix typos in documentation