-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DRAFT][luci/service] Support dynamic shape inference for reshape #13999
base: master
Are you sure you want to change the base?
Conversation
f699ab3
to
c9d5e68
Compare
3fce273
to
29ade8e
Compare
This commit supports dynamic shape inference for reshape operation ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>
29ade8e
to
40faded
Compare
TEST(ShapeRuleTest, reshape_by_input_const_static) | ||
TEST(ShapeRuleTest, reshape_by_circle_const) | ||
{ | ||
auto g = loco::make_graph(); | ||
auto node_reshape = g->nodes()->create<luci::CircleReshape>(); | ||
auto tensor_input = g->nodes()->create<luci::CircleInput>(); | ||
auto shape_by_input = g->nodes()->create<luci::CircleConst>(); | ||
auto shape_input = g->nodes()->create<luci::CircleConst>(); | ||
|
||
tensor_input->dtype(loco::DataType::S32); | ||
tensor_input->shape({2, 3, 4}); | ||
tensor_input->shape_status(luci::ShapeStatus::VALID); | ||
|
||
shape_by_input->dtype(loco::DataType::S32); | ||
shape_by_input->size<loco::DataType::S32>(2); | ||
shape_by_input->at<loco::DataType::S32>(0) = 6; | ||
shape_by_input->at<loco::DataType::S32>(1) = 4; | ||
shape_by_input->shape_status(luci::ShapeStatus::VALID); | ||
shape_input->dtype(loco::DataType::S32); | ||
shape_input->size<loco::DataType::S32>(2); | ||
shape_input->at<loco::DataType::S32>(0) = -1; | ||
shape_input->at<loco::DataType::S32>(1) = 4; | ||
shape_input->shape_status(luci::ShapeStatus::VALID); | ||
|
||
node_reshape->tensor(tensor_input); | ||
node_reshape->shape(shape_by_input); | ||
node_reshape->shape(shape_input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test modified?
If you'd like to add shape = circle_const(-1, 4)
case, Why didn't you add just new test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test modified?
If you'd like to add
shape = circle_const(-1, 4)
case, Why didn't you add just new test?
I tried to make all the cases to test "dynamic" shape inference so that it can be related to this PR's changes.
But the existed cases were supposed to test migration of the algorithm, so I think we can just leave it :)
I'll make the change to preserve existing cases.
* @note CircleReshape always has two inputs: `tensor` and `shape`. | ||
* The `shape` input can be CircleConst, CircleOutputDummy, or CircleNode. | ||
* - If the `shape` input is CircleConst, the shape is inferred from the constant. | ||
* - If the `shape` input is CircleOutputDummy, the shape is inferred from | ||
* the attribute if it exists. If the attribute does not exist, | ||
* the shape is inferred from the node iteself. | ||
* - If the `shape` input is CircleNode, the dynamic shape is propagated. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is there an easy way to apply these changes one by one?
By reading diff page (https://github.com/Samsung/ONE/pull/13999/files?diff=unified#diff-53743de477b450a036961928b169bb3cdbdfa63c96717ac0f9d88a5b3f696338R45-R52), It is hard to distinguish how each logics are updated. So, I'm afraid that it is hard to get reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK this is possible or not, but I'm trying to think a way to split this PR into small several PRs :
e.g.
- support CircleConst, not CircleConst case first
- apply should_infer
- support CircleDummy next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great! I'll work on it :)
if (const_shape_node != nullptr) | ||
bool should_infer = true; | ||
loco::TensorShape output_shape; | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below if-else statements can be little bit more simplified with following codes :)
if (auto const_shape = dynamic_cast<luci::CircleConst *>(node->shape()))
{
// inference with const_shape
}
else if (auto dummy_shape = dynamic_cast<luci::CircleOutputDummy *>(node->shape()))
{
// inference with attribute
}
else
{
auto node_shape = loco::must_cast<luci::CircleNode *>(node->shape());
// propagate dynamic shape
}
This commit supports dynamic shape inference for reshape operation
Dynamic shape inference algorithm
from: #13927 (comment), #13927 (comment)
CircleReshape
always has two inputs:tensor
andshape
.shape
input can beCircleConst
,CircleOutputDummy
, orCircleNode
.shape
input isCircleConst
shape
output_shape
would be staticreshape(tensor, [2, 3]) --> [2, 3]
shape
input isCircleOutputDummy
newShape
)shape
input), propagate the shape of node itselfshape
input isCircleNode
shape
input's size (because the exact shape cannot be determined at compile time)output_shape
would be dynamicreshape(tensor, shape_node(with size 3)) --> [?, ?, ?]
output_shape
and inputtensor
is fully known, that dimension is inferred by the count of the elementsWhen should not infer?
Unknown dimension of
output_shape
is NOT inferred (by theshould_infer
) when:shape
input isCircleNode
. Because if it's not constant, the only thing we can infer at compile time is the size of dimensions.tensor
input is not fully known. Because shape inference of reshape is based on the sameness of elements count.Remaining
own_shape
?from: #13935 (comment), #13935 (comment)
The usage of
circle_shape
might seems to be remaining ofown_shape
which should be removed by the new shape inference algorithm.First, this logic is used to support this recipe(
Reshape_003
) which has no attribute, no shape input.I've tried to revise this recipe according to this comment, and have read the related PRs (#1554, #1519).
But it was hard to make a policy about no attribute, no shape input case.
So, we decided not to fix another code(importer, recipes) and do it best in shape inference logic.
As a result, I followed the suggestion from #13927 (comment).
Neg test cases
from: #13977
Some of the neg test cases might not directly related to the changes that have been made in this PR.
(which means the
false return
andthrows
are not made inCircleReshape.cpp
.)Please tell me if these are inappropriate. I'll remove them and open an separate issue.
Related: #13927
Draft: #13935
Depends on: #13989
ONE-DCO-1.0-Signed-off-by: Jongwon Yang [email protected]