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

jongwonyang · 2024-09-11T07:41:30Z

This PR migrates reshape shape inference rule to sinf::Algorithm.

This PR is the first step of implementing dynamic shape inference algorithm for reshape operation.
Further implementation in the draft will be uploaded as a separate PR with detailed explanation after merging this PR.

Related: #13927
Draft: #13935

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

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

zetwhite · 2024-09-11T08:05:33Z

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

+TEST(ShapeRuleTest, reshape_input_tensor_undefined_NEG)
+{
+  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>();
+
+  tensor_input->dtype(loco::DataType::S32);
+  tensor_input->shape({2, 3, 4});
+  tensor_input->shape_status(luci::ShapeStatus::UNDEFINED);
+
+  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);
+
+  node_reshape->tensor(tensor_input);
+  node_reshape->shape(shape_by_input);
+
+  loco::TensorShape output_shape;
+  luci::sinf::Rule shape_inf_rule;
+
+  ASSERT_FALSE(shape_inf_rule.infer(node_reshape, output_shape));
+}
+
+TEST(ShapeRuleTest, reshape_input_shape_undefined_NEG)
+{
+  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>();
+
+  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::UNDEFINED);
+
+  node_reshape->tensor(tensor_input);
+  node_reshape->shape(shape_by_input);
+
+  loco::TensorShape output_shape;
+  luci::sinf::Rule shape_inf_rule;
+
+  ASSERT_FALSE(shape_inf_rule.infer(node_reshape, output_shape));
+}


I'm not sure adding these tests is good idea 🤔
Because in another PRs, we talked about not to add NEG tests in similar situations.
(related : #13977)

@shs-park Could you give an opinion about this?

I think so too.
Also, since the purpose of this PR is a simple migration, I don't think there's a need to include additional tests this time.

I think so too. Also, since the purpose of this PR is a simple migration, I don't think there's a need to include additional tests this time.

You mean, not just neg tests, but also all the tests are unnecessary? Because it's just migration?

For general (normal) cases, I think it's okay to leave it as is. 😅

Thanks for your opinion. Then, I'll remove the neg cases only :)

Thank you! And since we don't have enough NEG tests then, I think you could continue by creating a separate issue to add more NEG tests.

seanshpark · 2024-09-11T09:18:44Z

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

+      shape.rank(node->rank());
+      for (uint32_t r = 0; r < node->rank(); ++r)
+      {
+        // Shape inference rules in this file did not consider unknown dimension.


Suggested change

// Shape inference rules in this file did not consider unknown dimension.

// TODO remove this copy from `use_own(node);`

// Shape inference rules in this file did not consider unknown dimension.

like #13987 (review)

I have applied this suggestion. Thanks!

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

shs-park

LGTM!
=)

zetwhite · 2024-09-12T02:40:41Z

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

+std::ostream &operator<<(std::ostream &os, const loco::TensorShape &tensor_shape)
+{
+  os << "[";
+  for (uint32_t r = 0; r < tensor_shape.rank(); ++r)
+  {
+    if (r)
+      os << ",";
+
+    if (tensor_shape.dim(r).known())
+      os << tensor_shape.dim(r).value();
+    else
+      os << "?";
+  }
+  os << "]";
+  return os;
+}


(Not for this PR)

I find out that there are many std::ostream &operator<< overloadings for TensorShape in luci/services.

search operator<<(std::ostream &os, const loco::TensorShape in compiler/luci/service/
compiler/luci/service/src/CircleShapeInference.cpp: 30 31: std::ostream &operator<<(std::ostream &os, const loco::TensorShape &tensor_shape) 32 { compiler/luci/service/src/CircleShapeInferenceRule.cpp: 37 38: std::ostream &operator<<(std::ostream &os, const loco::TensorShape &tensor_shape) 39 { compiler/luci/service/src/Validate.cpp: 33 34: std::ostream &operator<<(std::ostream &os, const loco::TensorShape &tensor_shape) 35 {

How about introduce new file name compiler/luci/service/src/logHelper.h and making one implementation?

seanshpark

LGTM thank you!

seanshpark · 2024-09-12T03:42:50Z

Plz resolve conflicts

jongwonyang · 2024-09-12T03:56:01Z

Plz resolve conflicts

Resolved :)

shs-park

👍

seanshpark

LGTM thank you!

jongwonyang added PR/ready for review It is ready to review. Please review it. SSAFY labels Sep 11, 2024

jongwonyang requested review from a team September 11, 2024 07:41

seanshpark reviewed Sep 11, 2024

View reviewed changes

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

zetwhite reviewed Sep 11, 2024

View reviewed changes

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

zetwhite reviewed Sep 11, 2024

View reviewed changes

jongwonyang force-pushed the migrate_reshape branch from 436119f to 87e20be Compare September 11, 2024 08:08

seanshpark reviewed Sep 11, 2024

View reviewed changes

jongwonyang force-pushed the migrate_reshape branch 2 times, most recently from 6ecd2e4 to 82d2717 Compare September 11, 2024 11:50

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

82d2717

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

shs-park previously approved these changes Sep 11, 2024

View reviewed changes

shs-park requested review from zetwhite and seanshpark September 11, 2024 12:02

jongwonyang mentioned this pull request Sep 11, 2024

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

Closed

zetwhite reviewed Sep 12, 2024

View reviewed changes

zetwhite previously approved these changes Sep 12, 2024

View reviewed changes

seanshpark previously approved these changes Sep 12, 2024

View reviewed changes

Merge branch 'master' into migrate_reshape

d10a8bf

jongwonyang dismissed stale reviews from seanshpark, zetwhite, and shs-park via d10a8bf September 12, 2024 03:51

jongwonyang requested a review from seanshpark September 12, 2024 04:09

zetwhite approved these changes Sep 12, 2024

View reviewed changes

shs-park approved these changes Sep 12, 2024

View reviewed changes

seanshpark approved these changes Sep 12, 2024

View reviewed changes

seanshpark merged commit 5149447 into Samsung:master Sep 12, 2024
10 checks passed

jongwonyang mentioned this pull request Sep 12, 2024

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

Draft

jongwonyang mentioned this pull request Sep 20, 2024

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

Merged

jongwonyang deleted the migrate_reshape branch September 23, 2024 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jongwonyang commented Sep 11, 2024

zetwhite Sep 11, 2024

shs-park Sep 11, 2024

jongwonyang Sep 11, 2024

shs-park Sep 11, 2024

jongwonyang Sep 11, 2024

shs-park Sep 11, 2024

seanshpark Sep 11, 2024

jongwonyang Sep 11, 2024

shs-park left a comment

zetwhite Sep 12, 2024

seanshpark left a comment

seanshpark commented Sep 12, 2024

jongwonyang commented Sep 12, 2024

shs-park left a comment

seanshpark left a comment

	// Shape inference rules in this file did not consider unknown dimension.
	// TODO remove this copy from `use_own(node);`
	// Shape inference rules in this file did not consider unknown dimension.

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

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

Conversation

jongwonyang 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs-park left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanshpark left a comment

Choose a reason for hiding this comment

seanshpark commented Sep 12, 2024

jongwonyang commented Sep 12, 2024

shs-park left a comment

Choose a reason for hiding this comment

seanshpark left a comment

Choose a reason for hiding this comment