[DRAFT][luci/service] Support dynamic shape for reshape #13935

jongwonyang · 2024-09-04T11:56:09Z

This draft PR supports dynamic shape inference for reshape operation.

This draft PR consists of two parts.

Migrate reshape shape inference rule to sinf::Algorithm
Support dynamic shape inference for reshape

Dynamic shape inference algorithm

CircleReshape always has two inputs: tensor and shape.
- otherwise, throw an error
The shape input can be CircleConst, CircleOutputDummy, or CircleNode.
When the shape input is CircleConst
- The shape is inferred from the constant (use shape_by_input)
- output_shape would be static
- Example case : reshape(input, [2, 3]) --> [2, 3]
When the shape input is CircleOutputDummy
- First, try to shape inference using attribute
- Second, try to shape inference using node itself
When the shape input is CircleNode
- The shape is not inferred (meaning it only propagates based on shape_by_input)
- output_shape would be dynamic
- Example case : reshape(input, shape_node(with size 3)) --> [?, ?, ?]
If there is single unknown dimension in output_shape and input tensor is fully known, that dimension is inferred by the count of the elements

Related: #13927

ONE-DCO-1.0-Signed-off-by: Jongwon Yang [email protected]

compiler/luci/service/src/Nodes/CircleReshape.cpp

shs-park · 2024-09-05T11:17:24Z

FYI, you can add Reviewers all the SSAFY members like @Samsung/ssafy_2024 at once.

This commit migrates Reshape shape inference rule to sinf::Algorithm. ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>

This commit adds test cases for reshape operation. ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>

jongwonyang · 2024-09-10T04:53:33Z

@Samsung/ssafy_2024 PTAL :)

icodo98 · 2024-09-10T05:18:12Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+ */
+loco::TensorShape Algorithm::visit(const luci::CircleReshape *node)
+{
+  LOGGER(l);


Why do you need logger?

I think it could be removed. Thanks!

zetwhite · 2024-09-11T01:20:28Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+        if (node->newShape()->rank() > 0)
+        {
+          output_shape.rank(node->newShape()->rank());
+
+          for (uint32_t axis = 0; axis < output_shape.rank(); ++axis)
+          {
+            output_shape.dim(axis) = node->newShape()->dim(axis);
+          }
+        }


To check my understanding, This part corresponds to 'get shape from the attribute'. Right?

To check my understanding, This part corresponds to 'get shape from the attribute'. Right?

That is correct :)

zetwhite · 2024-09-11T01:21:20Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+        }
+        else
+        {
+          output_shape = circle_shape(node);


Also, to check my understanding, This part corresponds to 'get shape from own shape`.

Also, to check my understanding, This part corresponds to 'get shape from own shape`.

Yes, you're right :)

Why own_shape is remained?
I thought that this will be removed because we implemented more perfect inference logic..!
(ref : #13912 (comment))

Why own_shape is remained? I thought that this will be removed because we implemented more perfect inference logic..! (ref : #13912 (comment))

own_shape is remained to handle this recipe(Reshape_003) which has no attribute, no input shape.

I've read the related PRs (#1554, #1519) but I'm not sure how to handle this recipe.

Do you think we need to discuss about this recipe on #13927 ?

According to #13927 (comment), we may be able to revise the recipe first...!
@zetwhite How about your think? :)

@llFreetimell Thank you a lot for helping us 😄

I think @jongwonyang followed what i suggested in here : #13927 (comment)

regard Reshape X, attribute X a valid graph

while importing make Reshape/shape = DummyCircle

Since reshape IR allows only 2 inputs, there is no way to avoid creating a DummyCircle.

while shape inferencing if Reshape/shape = DummyCircle,

First, try to shape inference using attribute

Second, try to shape inference using output_shape

It was hard to make a policy about "no attribute, no shape input" case.
So, we try not to fix another code(importer, recipes) and do it best in shape inference logic.

So, we try not to fix another code(importer, recipes) and do it best in shape inference logic

I don't mean to say it should be done this way. For now, I chose an easy way.
If you have any other thoughts, please feel free to share it.

I understood :D
Then, I agree with your direction, let's go with it!

zetwhite · 2024-09-11T01:23:05Z

@Samsung/ssafy_2024 PTAL :)

I read the main shape infer logic and it looks good to me 😄

llFreetimell · 2024-09-11T05:09:33Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+
+      for (uint32_t axis = 0; axis < output_shape.rank(); ++axis)
+      {
+        output_shape.dim(axis) = const_input->at<S32>(axis);


If const_input has -1 value, the dimension value becomes 0xFFFFFFFF and known() == true.
When this happens, output_shape.dim(axis).unset() would be correct.

I missed it...!

Thanks for pointing really important thing out :)

I'll make the change.

llFreetimell · 2024-09-11T05:12:48Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+      const uint32_t dim_value = output_shape.dim(dim_index).value();
+      if (static_cast<int>(dim_value) == -1)


This is not recommended code :(
if (output_shape.dim(dim_index).known() == false) would be better

That is so true. Thank you!

I'll make the change.

shs-park · 2024-09-10T11:14:49Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+
+/**
+ * @note  CircleReshape always has two inputs: tensor and shape.
+ *        The shape input can be CircleConst, CircleOutputDummy, or CircleNode.


Question,
What does the CircleOutputDummy mean?

Question, What does the CircleOutputDummy mean?

While importing the circle file, CircleOutputDummy is added as node's shape input when there is no shape_by_input and no shape_by_attr.

ONE/compiler/luci/import/src/Nodes/CircleReshape.cpp

Lines 79 to 92 in 487afbd

auto *shape_node = (inputs.size() == 2) ? inputs.at(1) : nullptr;

if (shape_node == nullptr)

{

const auto *options = op.builtin_options.AsReshapeOptions();

if (options != nullptr)

shape_node = create_shape_node(options->new_shape, graph);

else

{

shape_node = graph->nodes()->create<CircleOutputDummy>();

shape_node->dtype(loco::DataType::S32);

shape_node->rank(0);

shape_node->name("Reshape/dummy");

}

}

shs-park · 2024-09-10T11:19:21Z

compiler/luci/service/src/Nodes/CircleReshape.cpp

+ *        - If the shape input is CircleOutputDummy, the shape is inferred from
+ *          the attribute or the node itself.


the attribute or the node itself

When does it refer to the attribute and when does it refer to the node itself?
Or, do attribute and node itself mean the same thing?

When does it refer to the attribute and when does it refer to the node itself? Or, do attribute and node itself mean the same thing?

If the shape input is CircleOutputDummy, it first checks if there's attribute (node->newShape()).

If there is attribute it uses attribute for shape inference.

If there is no attribute, then it uses shape of node iteself as a fallback behavior.

I've also changed this ambiguous comment :)

This commit supports dynamic shape inference for reshape operation ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>

jongwonyang requested review from seanshpark, llFreetimell, zetwhite, pcs1265, qsunki, icodo98, shs-park, kyeong8139 and Hanjin-Choi September 4, 2024 11:56

jongwonyang mentioned this pull request Sep 4, 2024

[compiler] Support dynamic shape for reshape #13927

Closed

zetwhite reviewed Sep 5, 2024

View reviewed changes

compiler/luci/service/src/Nodes/CircleReshape.cpp Show resolved Hide resolved

shs-park requested a review from a team September 5, 2024 11:17

jongwonyang force-pushed the draft_luci_reshape branch from 8fb5db6 to eab43e1 Compare September 9, 2024 02:11

jongwonyang added 2 commits September 9, 2024 12:55

[luci/service] Migrate Reshape shape inference rule to sinf::Algorithm

8a7d433

This commit migrates Reshape shape inference rule to sinf::Algorithm. ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>

[luci/service] Add test cases for reshape

436119f

This commit adds test cases for reshape operation. ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>

jongwonyang force-pushed the draft_luci_reshape branch 3 times, most recently from 7f05707 to 856794b Compare September 10, 2024 04:22

icodo98 reviewed Sep 10, 2024

View reviewed changes

jongwonyang force-pushed the draft_luci_reshape branch from 856794b to 51f79a2 Compare September 10, 2024 05:28

zetwhite reviewed Sep 11, 2024

View reviewed changes

jongwonyang changed the title ~~[DRAFT][WIP][luci/service] Support dynamic shape for reshape~~ [DRAFT][luci/service] Support dynamic shape for reshape Sep 11, 2024

llFreetimell reviewed Sep 11, 2024

View reviewed changes

jongwonyang force-pushed the draft_luci_reshape branch 2 times, most recently from 4001914 to 858f973 Compare September 11, 2024 06:35

jongwonyang mentioned this pull request Sep 11, 2024

[luci/service] Migrate reshape shape inference rule to sinf::Algorithm #13989

Merged

shs-park reviewed Sep 11, 2024

View reviewed changes

[luci/service] Support dynamic shape inference for reshape

0c2c5ac

This commit supports dynamic shape inference for reshape operation ONE-DCO-1.0-Signed-off-by: Jongwon Yang <[email protected]>

jongwonyang force-pushed the draft_luci_reshape branch from 858f973 to 0c2c5ac Compare September 11, 2024 08:31

jongwonyang mentioned this pull request Sep 11, 2024

[luci/service] Support dynamic shape inference for reshape jongwonyang/ONE#1

Closed

shs-park added the SSAFY label Sep 12, 2024

This was referenced Sep 12, 2024

[DRAFT][luci/service] Support dynamic shape inference for reshape #13999

Draft

[DRAFT][common-artifacts] Exclude Net_Gather_SparseToDense_AddV2_000 #14003

Closed

[DRAFT][luci/service] Enhance handling CircleConst as reshape's shape #14034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT][luci/service] Support dynamic shape for reshape #13935

[DRAFT][luci/service] Support dynamic shape for reshape #13935

jongwonyang commented Sep 4, 2024 •

edited

Loading

shs-park commented Sep 5, 2024

jongwonyang commented Sep 10, 2024

icodo98 Sep 10, 2024

jongwonyang Sep 10, 2024

zetwhite Sep 11, 2024

jongwonyang Sep 11, 2024

zetwhite Sep 11, 2024

jongwonyang Sep 11, 2024

llFreetimell Sep 11, 2024 •

edited

Loading

jongwonyang Sep 11, 2024 •

edited

Loading

llFreetimell Sep 11, 2024

zetwhite Sep 11, 2024

zetwhite Sep 11, 2024

llFreetimell Sep 11, 2024

zetwhite commented Sep 11, 2024

llFreetimell Sep 11, 2024

jongwonyang Sep 11, 2024

llFreetimell Sep 11, 2024

jongwonyang Sep 11, 2024

shs-park Sep 10, 2024

jongwonyang Sep 11, 2024 •

edited

Loading

shs-park Sep 10, 2024

jongwonyang Sep 11, 2024 •

edited

Loading

jongwonyang Sep 11, 2024

		const uint32_t dim_value = output_shape.dim(dim_index).value();
		if (static_cast<int>(dim_value) == -1)

	auto *shape_node = (inputs.size() == 2) ? inputs.at(1) : nullptr;
	if (shape_node == nullptr)
	{
	const auto *options = op.builtin_options.AsReshapeOptions();
	if (options != nullptr)
	shape_node = create_shape_node(options->new_shape, graph);
	else
	{
	shape_node = graph->nodes()->create<CircleOutputDummy>();
	shape_node->dtype(loco::DataType::S32);
	shape_node->rank(0);
	shape_node->name("Reshape/dummy");
	}
	}

		* - If the shape input is CircleOutputDummy, the shape is inferred from
		* the attribute or the node itself.

[DRAFT][luci/service] Support dynamic shape for reshape #13935

Are you sure you want to change the base?

[DRAFT][luci/service] Support dynamic shape for reshape #13935

Conversation

jongwonyang commented Sep 4, 2024 • edited Loading

Dynamic shape inference algorithm

shs-park commented Sep 5, 2024

jongwonyang commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llFreetimell Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

jongwonyang Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetwhite commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongwonyang Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongwonyang Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongwonyang commented Sep 4, 2024 •

edited

Loading

llFreetimell Sep 11, 2024 •

edited

Loading

jongwonyang Sep 11, 2024 •

edited

Loading

jongwonyang Sep 11, 2024 •

edited

Loading

jongwonyang Sep 11, 2024 •

edited

Loading