[luci] Use static dimensions from new_shape attribute by mbencer · Pull Request #14844 · Samsung/ONE

mbencer · 2025-03-13T12:18:56Z

This PR adds use static dimension from new_shape attribute even if only some of them are static and the rest dynamic.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com

jinevening · 2025-03-14T00:12:30Z

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

+  {
+    for (uint32_t axis = 0; axis < base_shape.rank(); ++axis)
+    {
+      if (!base_shape.dim(axis).known() && merged_shape.dim(axis).known())


This case handles shape_by_input has unknown dimension, while shape_by_attr knows the dimension. Could you give an example of this case?

Sure, in parallel with circle-resizer I'm working enabling LLama2 with dynamic input in circle-exir.

I have a case where target shape input is is non-const (not CircleConst) so in the moment of shape inference here we can calculate only output rank based on it. By the attribute = {-1, 256} I can include also one known dimension here which is very useful for further shape inference (before runtime execution).
Missing first dimension is calculated in runtime.

btw, I see that sth similar is already done but only for CircleOutputDummy but I am not sure why

Thaks for the explanation. So, this happens because circle-resizer only updates input shape but not attributes.

I'm working enabling LLama2 with dynamic input in circle-exir.

Could you explain it a little bit more? I imagined that it can be used for changing sequence length as below.

Torch -> static circle (seq len: 256) -> static circle (seq len: 512 or else)

But it seems that your model includes dynamic shapes. Could you describe your usage scenario?

Sure. In general I'm trying the resolve the issue 293 from circle-exir repo.

I exported LLama2 model with dynamic batch and sequence length using my circle-exir draft PR 871.

I resized the model to static shapes batch: 1 and seq_len: 3 using my draft tool circle-resizer.

It fails during luci::CircleShapeInferencePass without this change in the current moment.

Thanks. So, the whole process is Torch -> dynamic shape circle (batch and seq_len are unknown) -> static shape circle.

Dynamic shape circle would have additional operators, such as Shape, not executable on NPU. circle-resizer may only update the shapes while not removing those operators. Do you have a plan to remove them as well, e.g., using constant folding?

Dynamic shape circle would have additional operators, such as Shape, not executable on NPU. circle-resizer may only update the shapes while not removing those operators. Do you have a plan to remove them as well, e.g., using constant folding?

You are completely right. The next step should be calling optimization passes (like ConstantFold - especially Shape->Gather patterns). In the ideal scenario the model after such running such passes should be the same as a static version produced by frontend. However, as I understand it should be rather a job for circle2circle

jinevening · 2025-03-14T00:13:24Z

You may need to change the commit message. [luci-interpreter] -> [luci]

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

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

mbencer · 2025-03-14T12:59:47Z

btw, it's rather a topic for a separate PR but I think that is_static_shape can depend only on input data shape. In the current moment, it's also set to false for dynamic dimensions in newShape attribute (but no for targert_shape input)🤔

mbencer · 2025-03-14T13:00:43Z

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

    }
    if (unknown_dim_index != UINT32_MAX)
    {
+      if (input_element_count % output_element_count != 0)


For negative tests purpose, should be moved to a separate PR?

Before asking, how is this condition related with your issue?

I can't catch the problem.

It's not directly related with my change but I haven't better ideas to new negative tests (especially because we have in Reshape shape infer impl rather asserts instead of throwing exceptions).
But I see now, that it's better to move it to a separate PR ;)

seanshpark · 2025-03-16T22:26:38Z

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

+namespace
+{
+loco::TensorShape merge_shapes(const loco::TensorShape &base_shape,


Suggested change

namespace

{

loco::TensorShape merge_shapes(const loco::TensorShape &base_shape,

namespace

{

loco::TensorShape merge_shapes(const loco::TensorShape &base_shape,

seanshpark · 2025-03-16T22:33:53Z

@mbencer , I can't see what case you are solving with Reshape Op.
To understand better, (1) prepare a draft with recipes in /res/TensorFlowLiteRecipes with problamatic models.
(2) add comments in the code or in the PR so that reviewers can understand.
(3) add to circle2circle-dredd-recipe-test for testing

mbencer · 2025-03-18T16:15:41Z

@mbencer , I can't see what case you are solving with Reshape Op. To understand better, (1) prepare a draft with recipes in /res/TensorFlowLiteRecipes with problamatic models. (2) add comments in the code or in the PR so that reviewers can understand. (3) add to circle2circle-dredd-recipe-test for testing

Unfortunately, I have a problem with preparing such test model using tensor flow recipe. If I try to set dynamic dimension (by -1) in such recipe, it fails. It looks like there is a special field shape_signature for dynamic shape purposes but it looks like sth dedicated for TF format and not handled in luci/service properly.

I've prepared such (simplified) test model using CircleGen (dumped to a file) -
reshape_squeeze.zip

Without this PR, shape inference pass fails during Squeeze inference because dynamic dimension in input shape is not checked. But even if we add checking dyn dimension in Squueze, the shape inference doesn't look good:

I am not sure what happens in the version without Reshape change from this PR. I'll try to debug it.

seanshpark · 2025-03-18T21:19:10Z

Why are you struggling with Squeeze if you are working with Reshape ?

jinevening · 2025-03-19T00:33:10Z

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

    {
+      if (input_element_count % output_element_count != 0)
+      {
+        INTERNAL_EXN("Unknown output dimension cannot be calculated for inputs");


Suggested change

INTERNAL_EXN("Unknown output dimension cannot be calculated for inputs");

INTERNAL_EXN("Reshape Op cannot infer unknown dimension from inputs.");

jinevening · 2025-03-19T00:34:18Z

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

  ASSERT_EQ(4, output_shape.dim(1).value());
 }

+TEST(ShapeRuleTest, reshape_should_infer_incorrect_zero_NEG)


Suggested change

TEST(ShapeRuleTest, reshape_should_infer_incorrect_zero_NEG)

TEST(ShapeRuleTest, reshape_zero_rank_mismatch_NEG)

jinevening · 2025-03-19T00:34:53Z

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

+  ASSERT_THROW(shape_inf_rule.infer(node_reshape, output_shape), oops::InternalExn);
+}
+
+TEST(ShapeRuleTest, reshape_should_infer_incorrect_target_shape_NEG)


Suggested change

TEST(ShapeRuleTest, reshape_should_infer_incorrect_target_shape_NEG)

TEST(ShapeRuleTest, reshape_wrong_target_shape_NEG)

jinevening · 2025-03-19T00:38:15Z

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

+  ASSERT_TRUE(shape_inf_rule.infer(node_reshape, output_shape));
+
+  ASSERT_EQ(2, output_shape.rank());
+  ASSERT_FALSE(output_shape.dim(0).known());


Why is this unknown? I guess that Reshape (2,3,4) into (-1, 12) should have returned (2, 12)?

Hmm, good point. My intention here was to set as dynamic, but you are right that -1 have also special meaning.
I debug it more deeply and IHMO is_static_shape = false is not needed here. As a result -1 handling here is not called.

But it seems to be sth separately to the main goal of my change (especially if even now it's not very clear😉)

To avoid confusions I've changed the test data.

mbencer · 2025-03-19T07:25:35Z

Why are you struggling with Squeeze if you are working with Reshape ?

As I mentioned in #14844 (comment) - without fix from this PR in Reshape, the shape inference fails on Squeeze op (which is after Reshape).
In version without fix from this PR + quick temporary fix in Squeeze the propagated shapes seems to be incorrect - I don't know yet now why ;)

EDIT: I've found the reason of strange shape inference results in version without this PR + fix in Squeeze.
luci/service stores dynamic dimensions as 0. For some reasons Netron shows "0" dim as 1 🤔

btw: IMO storing dynamic dimensions as 0 is not the best choice since 0 have special meaning for some cases like 0 in Reshape target shape. The better choice seems to be -1 - I am also using such convention in circle-resizer

mbencer · 2025-03-19T18:25:48Z

@seanshpark @jinevening

I've tried to clean up the stuff by:
-> In this PR I am introducing only using static dimension from newShape attribute even if some of dimensions are dynamic (if shape input is non-const). I've skipped new merge_shapes method and used existing impl (before called only for CircleOutputDummy)
-> problem discussed here about Squeeze (which fails in my model without Reshape fix) moved to: #14857
-> new negative tests (+ additional verification) moved to: #14858 and #14859
I've tried to include there each of your review requests.

IHMO still sth work to consider: how to mark dynamic dimensions: 0, -1 or sth else?

This PR adds use static dimension from new_shape attribute even if only some of them are static and the rest dynamic. ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com

seanshpark · 2025-03-19T21:08:43Z

Plz add a draft link to see #14844 (comment) for this too

seanshpark · 2025-03-19T21:41:28Z

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

      auto rank = new_shape->rank();
-      auto shape_dummy = dynamic_cast<luci::CircleOutputDummy *>(node->shape());
-      if (shape_dummy && rank > 0)
+      if (rank > 0)


Q) why did you remove to check shape_dummy ?

For my model converted from Pytorch I would like to use dimensions stored in newShape but models converted from Pytorch have no luci::CircleOutputDummy

Do you know what and how CircleOutputDummy works?

I know only (based on the spec) that it's "Temporary DummyNode used with dangle CircleNode" but to be honest I don't know why it is used here ;)

If you don't know the purpose of this node, why did you deleted it?

Just for clarification - I believe I understand concept of CircleOutputDummy here -> we are using values from attribute only if shape input to missing (dummy node assign to avoid dangling operand) -> attribute here is to reflect TF impl of Reshape node.

I am just not understand why we cannot relax the algorithm -> if based on shape input we can only calculate rank, let's check if maybe we can extract sth valuable from attribute (still treating input shape with the higher priority)

seanshpark · 2025-03-19T21:42:20Z

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

+  auto node_reshape = g->nodes()->create<luci::CircleReshape>();
+  auto tensor_input = g->nodes()->create<luci::CircleInput>();
+  auto target_shape = g->nodes()->create<luci::CircleInput>();
+  ;


Suggested change

;

thank you for catching it. removed

seanshpark · 2025-03-19T22:24:04Z

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

+      if (node->newShape()->dim(axis) > 0)
+      {
+        shape_by_attr.dim(axis) = node->newShape()->dim(axis);
+      }
+      else
+      {
+        shape_by_attr.dim(axis).unset(); // unset means unknown dimension
+      }


how is this change related with removing shape_dummy ?

The purpose of the change is to align way of dynamic shape representation: -1 for newShape and 0 for shape_by_attr.
But relation with shape_dummy removing is only to not show info log about shape mismatch.

is only to not show info log about shape mismatch.

I don't understand this. plz explain your understandings about this.

Let's consider even my case from reshape_by_newShape_dynamic unit test:
shape_by_input here after

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

Lines 130 to 135 in e5e151d

if (new_shape->dim(i) > 0)

shape_by_input.dim(i) = static_cast<uint32_t>(new_shape->dim(i));

else

{

is_static_shape = false;

shape_by_input.dim(i).unset();

is [0,2,0]
while shape_by_attr is [-1,2,-1]
TensorShape::operator== doesn't handle -1 as dynamic dimension

doesn't handle -1 as dynamic dimension

Not this. I'm asking about the relation with shape_dummy removal.
I understand about handling 0 to be handled as unknown.

And I really don't understand why you are mentioning about the log.
What is the problem with the log?

IMO incorrect information about mismatch will be shown just because other representation of dynamic dimension. It is related with this change because after call

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

Lines 130 to 135 in e5e151d

if (new_shape->dim(i) > 0)

shape_by_input.dim(i) = static_cast<uint32_t>(new_shape->dim(i));

else

{

is_static_shape = false;

shape_by_input.dim(i).unset();

(which is possible because of removing shape_dummy) condition shape_by_input == shape_by_attr will be negative incorrectly

seanshpark · 2025-03-19T22:28:14Z

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

+  ASSERT_TRUE(output_shape.dim(1).known());
+  ASSERT_EQ(4, output_shape.dim(1).value());
+  ASSERT_FALSE(output_shape.dim(2).known());
+}


IIUC, you are to reshape (2, 3, 4) to (-1, 4, -1)
I understand you are just preparing a test but
I really don't understand how (2, 3, 4) can be reshaped to (-1, 4, -1).
This values can make others reading this code confused about the ReshapeOp.
If you think I don't know about this, please explain.

it's probably can be reshaping to (6, 4, 1) but agree that such test data can be confusing. Changed to 2 (still with assumption that -1 means dyn dimension)

Have you tried running with pytorch or TF with reshape from (2, 3, 4) to (6, 4, 1) ?
(and maybe with (-1, 4, -1))

import tensorflow as tf input = tf.zeros([2, 3, 4]) print(tf.reshape(input, [6, 4, 1]).shape) import torch input = torch.zeros(2, 3, 4) print(torch.reshape(input, (6, 4, 1)).shape)

for both (6, 4, 1)

seanshpark · 2025-03-20T09:20:12Z

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

+  // reshape to {dynamic, 4, dynamic}
+  node_reshape->newShape()->rank(3);
+  node_reshape->newShape()->dim(0) = -1;
+  node_reshape->newShape()->dim(1) = 2;


Now you changed 4 to 2. What's the rationale behind this?

the idea was to set data easier to understand (especially after shape specialization)

seanshpark · 2025-03-20T09:39:47Z

For my model converted from Pytorch I would like to use dimensions stored in newShape but models converted from Pytorch have no luci::CircleOutputDummy

I really think this is VERY dangerous way of thinking. We also support tflite and TensorFlow files.
Do you think you haven't met the situation so that you can ignore existing codes? Wow...

mbencer · 2025-03-20T10:24:19Z

For my model converted from Pytorch I would like to use dimensions stored in newShape but models converted from Pytorch have no luci::CircleOutputDummy

I really think this is VERY dangerous way of thinking. We also support tflite and TensorFlow files. Do you think you haven't met the situation so that you can ignore existing codes? Wow...

I cannot at least now image case when such change can break something for other format.
But I agree that I am also not able to prove that the change doesn't break something

mbencer · 2025-03-20T10:29:58Z

Due to fact that I have no strong arguments that the change is completely safe for other frontends I'm closing the PR for now. For my current purposes (LLama2 with dynamic batch and seq_len) is enough to introduce #14857 (not all possible dimensions will be propagated during shape inference) but circle-interpreter is still able to inference the model.

mbencer requested a review from a team March 13, 2025 12:18

mbencer mentioned this pull request Mar 12, 2025

[circle-resizer] Implementation a new tool for model resizing #14791

Open

8 tasks

jinevening reviewed Mar 14, 2025

View reviewed changes

seanshpark reviewed Mar 14, 2025

View reviewed changes

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

seanshpark reviewed Mar 14, 2025

View reviewed changes

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

seanshpark reviewed Mar 14, 2025

View reviewed changes

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

mbencer commented Mar 14, 2025

View reviewed changes

mbencer requested review from jinevening and seanshpark March 14, 2025 13:01

mbencer force-pushed the mbencer/ReshapeShapeInfer branch from 442cf87 to d57f2e7 Compare March 14, 2025 13:03

mbencer changed the title ~~[luci-interpreter] Merge target shape from input node and attribute~~ [luci] Merge target shape from input node and attribute Mar 14, 2025

seanshpark reviewed Mar 16, 2025

View reviewed changes

jinevening reviewed Mar 19, 2025

View reviewed changes

[luci] Use static dimensions from new_shape attribute

b4da310

This PR adds use static dimension from new_shape attribute even if only some of them are static and the rest dynamic. ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com

mbencer force-pushed the mbencer/ReshapeShapeInfer branch from d57f2e7 to b4da310 Compare March 19, 2025 18:28

mbencer changed the title ~~[luci] Merge target shape from input node and attribute~~ [luci] Use static dimensions from new_shape attribute Mar 19, 2025

mbencer requested review from jinevening and seanshpark March 19, 2025 18:29

seanshpark mentioned this pull request Mar 19, 2025

[luci] Handle dynamic dimensions in Squeeze shape inference #14857

Merged

This was referenced Mar 19, 2025

[luci] Add additional negative unit test for Reshape #14859

Merged

[luci] Add additional check of unknown dimension for Reshape #14858

Merged

seanshpark reviewed Mar 19, 2025

View reviewed changes

review remarks

d3b46c2

seanshpark reviewed Mar 20, 2025

View reviewed changes

mbencer closed this Mar 20, 2025

mbencer deleted the mbencer/ReshapeShapeInfer branch May 2, 2025 08:12

	INTERNAL_EXN("Unknown output dimension cannot be calculated for inputs");
	INTERNAL_EXN("Reshape Op cannot infer unknown dimension from inputs.");

	TEST(ShapeRuleTest, reshape_should_infer_incorrect_zero_NEG)
	TEST(ShapeRuleTest, reshape_zero_rank_mismatch_NEG)

	TEST(ShapeRuleTest, reshape_should_infer_incorrect_target_shape_NEG)
	TEST(ShapeRuleTest, reshape_wrong_target_shape_NEG)

	if (new_shape->dim(i) > 0)
	shape_by_input.dim(i) = static_cast<uint32_t>(new_shape->dim(i));
	else
	{
	is_static_shape = false;
	shape_by_input.dim(i).unset();

Conversation

mbencer commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbencer Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinevening Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinevening commented Mar 14, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbencer commented Mar 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanshpark commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbencer commented Mar 18, 2025

Uh oh!

seanshpark commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinevening Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbencer commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbencer commented Mar 19, 2025

Uh oh!

seanshpark commented Mar 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbencer Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mbencer commented Mar 13, 2025 •

edited

Loading

mbencer Mar 14, 2025 •

edited

Loading

jinevening Mar 19, 2025 •

edited

Loading

seanshpark commented Mar 16, 2025 •

edited

Loading

jinevening Mar 19, 2025 •

edited

Loading

mbencer commented Mar 19, 2025 •

edited

Loading

mbencer Mar 20, 2025 •

edited

Loading

seanshpark Mar 19, 2025 •

edited

Loading

seanshpark Mar 20, 2025 •

edited

Loading