[compiler] Support dynamic shape for reshape #13927

zetwhite · 2024-09-04T05:02:11Z

origin: [luci] Shape inference for operators whose inputs have dynamic shape #13697
first try by @jongwonyang, [luci/service] Infer dynamic shape for constant reshape operation #13912

While reviewing above PR, I think we need to make some consensus about reshape shape inference rule.
So, I make this issue to discuss details.

I'd like to suggest below 2 things. If you have some free time, please take a look and leave any kind of comments!

/cc @llFreetimell @jongwonyang @shs-park @seanshpark

The text was updated successfully, but these errors were encountered:

zetwhite · 2024-09-04T07:00:03Z

Suggestion 2

About reshape shape inference rules, I'm considering following the rules.

Let's just forward reshape/shape into the output_shape.
- _{reshape/shape is a TensorShape - decided by a data of CircleConst or a tensor from CircleNode}
If output_shape has 'one unknown dim' and it is possible to infer the unknown dim, infer it.
- otherwise, just return the unknown dimensions as they are.

I think this rule is simple and we don't need discussions, But I leave it to ensure that we have a same understanding.

zetwhite · 2024-09-04T07:14:52Z

It is cumbersome to determine when to use shape_by_attr and when to use shape_by_input, while shape inferencing.

@llFreetimell suggest to follow tensorflow rules

use shape_by_attr by default
if shape_by_attr not exists, use shape_by_input
refer / https://github.com/tensorflow/tensorflow/blob/2fe29379f6449d263b787eb636020fd81017b8bf/tensorflow/lite/kernels/reshape.cc#L137-L143

I think this is also a good idea :)

BTW, other current shape inference use shape_by_input and ignore shape_by_attr.
It just prints a log if shape_by_input != shape_by_attr

ONE/compiler/luci/service/src/CircleShapeInferenceRule.cpp

Line 1047 in 949661a

loco::TensorShape output_shape = shape_by_input;

llFreetimell · 2024-09-04T07:51:42Z

@llFreetimell suggest to follow tensorflow rules

Due to my wrong comments, some misunderstanding is propagated 😅
My suggestion was following :

When reshape/shape is CircleConst
- use shape_by_input & output_shape would be static
- Example case : reshape(input, [2, 3]), attr=[2,3] --> [2, 3]
When reshape/shape is CircleNode
- use shape_by_input & output_shape would be dynamic
- Example case : reshape(input, shape_node) --> [?, ?, ?]
When reshape/shape does not exist
- use shape_by_attr & output_shape would be static
- Example case : reshape(input) --> [shape_by_attr]

For me, suggestion 1 and 2 are all fine :)
However, if suggestion 2 is selected, I hope that above modification is applied T.T

jongwonyang · 2024-09-04T08:23:49Z

When reshape/shape does not exist

For me, suggestion 1 and 2 are all fine :) However, if suggestion 2 is selected, I hope that above modification is applied T.T

I have a small question.

If my understanding below is correct, does this modification involve changing the base class of the CircleReshape node?

Current implementation always adds shape_by_input even it's not provided. (based on shape_by_attr)

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

Line 77 in 949661a

    
           // If the second input is not provided, generate it based on the value of the attribute.

The comment says it's because of the current requirement of the IR(CircleReshape).

I think it's because current CircleReshape node is based on FixedArityNode.

ONE/compiler/luci/lang/include/luci/IR/Nodes/CircleReshape.h

Line 31 in 433edf9

    
           class CircleReshape final : public FixedArityNode<2, CircleNodeImpl<CircleOpcode::RESHAPE>>

zetwhite · 2024-09-04T08:44:32Z

Due to my wrong comments, some misunderstanding is propagated 😅

Ah, I misunderstood tensorflow codes 😢 Sorry for confusion

My suggestion was following :

Thank you for detailed guide :) It looks good to me.

llFreetimell · 2024-09-04T08:49:41Z

does this modification involve changing the base class of the CircleReshape node?

Not at all :)

Current implementation always adds shape_by_input even it's not provided. (based on shape_by_attr)

I missed this codes exist... thanks..!
@zetwhite doesn't it seems some kinds of canonicalization logic...?
If so, how about following re-suggestions?

suggestion 1 with moving that logic from luci/import to canonicalize pass
suggestion 2 with checking reshape/shape is CircleOutputDummy or not

If I am misunderstanding something, please let me know :(

zetwhite · 2024-09-04T08:56:25Z

@zetwhite doesn't it seems some kinds of canonicalization logic...?

I think so. Hmm..
IMHO, moving it to canonicalization is a bit out of scope, we could talk about it later (or another issue).

Current implementation always adds shape_by_input even it's not provided. (based on shape_by_attr)

I didn't know it. thank you for figuring it out 👍

If so, how about this? A bit modified version from @llFreetimell

make sure that CircleReshape has always reshape/shape
- otherwise, throw an error
When reshape/shape is CircleConst
- use shape_by_input & output_shape would be static
- Example case : reshape(input, [2, 3]) --> [2, 3]
When reshape/shape is CircleNode
- use shape_by_input & output_shape would be dynamic
- Example case : reshape(input, shape_node) --> [?, ?, ?]
When reshape/shape is CircleOutputDummy
- reshape without shape might mean the graph is invalid -> so, let's throw an error in this case.

llFreetimell · 2024-09-04T09:19:09Z

@zetwhite Looks nice :D I agree for your final suggestion!

zetwhite · 2024-09-04T10:54:56Z

@jongwonyang Could you make a draft PR based on this discussion?
If sth is ambiguous while working, please freely leave any comment here :)

+) I'll take a look this recipe ( #13866 (comment) ) and try to fix it.

jongwonyang · 2024-09-04T11:05:02Z

@jongwonyang Could you make a draft PR based on this discussion? If sth is ambiguous while working, please freely leave any comment here :)

Of course! I'll create the draft PR ASAP. The final suggestion looks good to me :)

Thank you for addressing this issue!

+) I'll take a look this recipe ( #13866 (comment) ) and try to fix it.

Thank you for this too :)

jongwonyang · 2024-09-04T12:16:40Z

I have created the draft PR #13935

seanshpark · 2024-09-04T21:36:48Z

When reshape/shape is CircleOutputDummy
reshape without shape might mean the graph is invalid -> so, let's throw an error in this case.

No. This is not invalid. We had a model like this.
We should use newShape attribute.

jongwonyang · 2024-09-05T04:35:33Z

No. This is not invalid. We had a model like this. We should use newShape attribute.

Thanks for pointing out.

But the current CircleReshapeGraphBuilder creates CircleOutputDummy when there's no shape_by_input and no shape_by_attr.

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

Line 87 in 487afbd

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

Could you give me some more detailed explanation about how to utilize newShape attribute of CircleReshape?

seanshpark · 2024-09-05T04:54:31Z

creates CircleOutputDummy when there's no shape_by_input and no shape_by_attr.

Can you please explain how to got this from the code ?

zetwhite · 2024-09-05T06:05:45Z

creates CircleOutputDummy when there's no shape_by_input and no shape_by_attr.

Can you please explain how to got this from the code ?

I guess it from the luci/import code. Is there a thing I missed?

copy from luci/import/src/Nodes/CircleReshape.cpp

  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 X, attribute O -> create input node based on attribute
      shape_node = create_shape_node(options->new_shape, graph);
	} 
    else
    { 
	  // shape_node X, attribute X -> create CircleOutputDummy 
	  // If CircleOutputDummy exist, it means that both shape_node and attribute are NOT exist 
      shape_node = graph->nodes()->create<CircleOutputDummy>();
      shape_node->dtype(loco::DataType::S32);
      shape_node->rank(0);
      shape_node->name("Reshape/dummy");
    }
  }

zetwhite · 2024-09-05T06:27:58Z

+) Ah, Maybe this situation can happened.

Above luci/import loads a circle file into memory.

If a circle file has a CircleReshape (input = CircleDummy, attirbute=[1, 2, 3]), luci/import might just pass it.
So, luci/pass could meet "CircleDummy O, Attribute O" situation.

llFreetimell · 2024-09-05T06:46:29Z

If a circle file has a CircleReshape (input = CircleDummy, attirbute=[1, 2, 3]), luci/import might just pass it.
So, luci/pass could meet "CircleDummy O, Attribute O" situation.

Isn't CircleReshape (input = CircleDummy, attirbute=[1, 2, 3]) equal to shape_node X, attribute O situation..?

zetwhite · 2024-09-05T07:00:32Z

Isn't CircleReshape (input = CircleDummy, attirbute=[1, 2, 3]) equal to shape_node X, attribute O situation..?

CircleReshape(shape_node X, attirbute O) -> luci/import -> CircleReshape(shape_node=CircleConst, attribute O)
- shape_node will be generated base on attribute values
CircleReshape(shape_node = CircleDummy, attirbute O) -> luci/import -> CircleReshape(shape_node = CircleDummy, attribute O)
- reshape node with dummy input, but valid attribute might appeared

BTW, I don't know that CircleReshape(shape_node = CircleDummy, attribute O) can be generated from tflite2circle or onnx2circle.

seanshpark · 2024-09-05T07:08:06Z

I think the current build() code can give some confusion, and also my memories are also not clear.
Last related change is #1519 , where creation of CircleDummy was introduced.
We have #1554 as regression test, read/write and shape inference can be checked.

Not sure how to define valid/invalid models now, but we can have

Reshape/shape O, attribute O
Reshape/shape O, attribute X
Reshape/shape X, attribute O

Not sure Reshape/shape X, attribute X can exist in real world.

As current build() logic can give misunderstandings, we could revise current implementation.

zetwhite · 2024-09-05T07:56:19Z

We have #1554 as regression test, read/write and shape inference can be checked.
Not sure Reshape/shape X, attribute X can exist in real world.

Thanks for information.

Since there is a recipe Reshape/shape X, attribute X, we have to decide how to handle this case.

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
regard Reshape/shape X, attribute X an invalid graph
- remove Reshape/shape X, attribute X recipe
- throw an error in build()
- (also, throw an error in shape inference)

seanshpark · 2024-09-05T08:00:12Z

there is a recipe Reshape/shape X, attribute X

Can you give the recipe name?

zetwhite · 2024-09-05T08:01:55Z

Can you give the recipe name?

I was considering this recipe.

https://github.com/seanshpark/ONE/blob/ae0b9c5816dbc3a316e94a39797aae359078b846/res/TensorFlowLiteRecipes/Reshape_003/test.recipe

seanshpark · 2024-09-05T08:04:11Z

From history, #1519 , we had a real world model for both X.
But actually, comment says

There is a model where shape input is not Constant

We can remove 003 and add 004 for non constant input, like simple Add.

glistening · 2024-09-05T08:28:14Z

I don't know the details and history of frontend compiler.

But you may refer to tflite's behavior.

https://github.com/tensorflow/tensorflow/blob/885b3d6a4091e5610a6b342e802e1850bd82a5e1/tensorflow/lite/kernels/reshape.cc#L145-L176

TfLiteStatus Prepare(TfLiteContext* context, TfLiteNode* node) {
  TF_LITE_ENSURE(context, NumInputs(node) == 1 || NumInputs(node) == 2);
  TF_LITE_ENSURE_EQ(context, NumOutputs(node), 1);
  OpData* op_data = reinterpret_cast<OpData*>(node->user_data);
  op_data->output_ptr = nullptr;

  // Always postpone sizing string tensors, even if we could in principle
  // calculate their shapes now. String tensors don't benefit from having their
  // shapes precalculated because the actual memory can only be allocated after
  // we know all the content.
  TfLiteTensor* output;
  TF_LITE_ENSURE_OK(context,
                    GetOutputSafe(context, node, kOutputTensor, &output));
  if (output->type != kTfLiteString) {
    const TfLiteTensor* input = GetInput(context, node, kInputTensor);
    const TfLiteTensor* shape = GetInput(context, node, kShapeTensor);
    if (NumInputs(node) == 1 || IsConstantOrPersistentTensor(shape)) {
      if (IsConstantOrPersistentTensor(input)) {
        SetTensorToPersistentRo(output);
        TF_LITE_ENSURE_OK(context, ResizeOutput(context, node));
        op_data->output_ptr = output->data.data;
        memcpy(output->data.data, input->data.data, input->bytes);
        return kTfLiteOk;
      } else {
        TF_LITE_ENSURE_OK(context, ResizeOutput(context, node));
      }
    } else {
      SetTensorToDynamic(output);
    }
  }
  return kTfLiteOk;
}

jongwonyang · 2024-09-05T11:34:56Z

Sorry for the late response.

I completely misunderstood the role of luci/import. Apologies for the confusion.

Now, I have a better understanding of the implementation, taking into account the existence of Reshape/shape and attribute.

I'll be adding more details to the draft PR soon.

Thanks again for your participation and the wonderful discussions!

This comment was marked as resolved.

Sign in to view

jongwonyang added the SSAFY label Sep 4, 2024

jongwonyang mentioned this issue Sep 4, 2024

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

Draft

This was referenced Sep 11, 2024

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

Merged

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

Closed

This was referenced Sep 19, 2024

[DRAFT][luci/service] Support CircleOutputDummy as reshape's shape jongwonyang/ONE#3

Draft

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

Open

[DRAFT][luci/service] Support CircleNode as reshape's shape jongwonyang/ONE#4

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Support dynamic shape for reshape #13927

[compiler] Support dynamic shape for reshape #13927

zetwhite commented Sep 4, 2024 •

edited

Loading

This comment was marked as resolved.

zetwhite commented Sep 4, 2024 •

edited

Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

llFreetimell commented Sep 4, 2024

jongwonyang commented Sep 4, 2024

zetwhite commented Sep 4, 2024

llFreetimell commented Sep 4, 2024 •

edited

Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

llFreetimell commented Sep 4, 2024

zetwhite commented Sep 4, 2024 •

edited

Loading

jongwonyang commented Sep 4, 2024

jongwonyang commented Sep 4, 2024

seanshpark commented Sep 4, 2024

jongwonyang commented Sep 5, 2024 •

edited

Loading

seanshpark commented Sep 5, 2024

zetwhite commented Sep 5, 2024 •

edited

Loading

zetwhite commented Sep 5, 2024 •

edited

Loading

llFreetimell commented Sep 5, 2024

zetwhite commented Sep 5, 2024 •

edited

Loading

seanshpark commented Sep 5, 2024

zetwhite commented Sep 5, 2024 •

edited

Loading

seanshpark commented Sep 5, 2024

zetwhite commented Sep 5, 2024

seanshpark commented Sep 5, 2024 •

edited

Loading

glistening commented Sep 5, 2024

jongwonyang commented Sep 5, 2024 •

edited

Loading

[compiler] Support dynamic shape for reshape #13927

[compiler] Support dynamic shape for reshape #13927

Comments

zetwhite commented Sep 4, 2024 • edited Loading

This comment was marked as resolved.

zetwhite commented Sep 4, 2024 • edited Loading

Suggestion 2

zetwhite commented Sep 4, 2024 • edited Loading

llFreetimell commented Sep 4, 2024

jongwonyang commented Sep 4, 2024

zetwhite commented Sep 4, 2024

llFreetimell commented Sep 4, 2024 • edited Loading

zetwhite commented Sep 4, 2024 • edited Loading

llFreetimell commented Sep 4, 2024

zetwhite commented Sep 4, 2024 • edited Loading

jongwonyang commented Sep 4, 2024

jongwonyang commented Sep 4, 2024

seanshpark commented Sep 4, 2024

jongwonyang commented Sep 5, 2024 • edited Loading

seanshpark commented Sep 5, 2024

zetwhite commented Sep 5, 2024 • edited Loading

zetwhite commented Sep 5, 2024 • edited Loading

llFreetimell commented Sep 5, 2024

zetwhite commented Sep 5, 2024 • edited Loading

seanshpark commented Sep 5, 2024

zetwhite commented Sep 5, 2024 • edited Loading

seanshpark commented Sep 5, 2024

zetwhite commented Sep 5, 2024

seanshpark commented Sep 5, 2024 • edited Loading

glistening commented Sep 5, 2024

jongwonyang commented Sep 5, 2024 • edited Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

llFreetimell commented Sep 4, 2024 •

edited

Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

zetwhite commented Sep 4, 2024 •

edited

Loading

jongwonyang commented Sep 5, 2024 •

edited

Loading

zetwhite commented Sep 5, 2024 •

edited

Loading

zetwhite commented Sep 5, 2024 •

edited

Loading

zetwhite commented Sep 5, 2024 •

edited

Loading

zetwhite commented Sep 5, 2024 •

edited

Loading

seanshpark commented Sep 5, 2024 •

edited

Loading

jongwonyang commented Sep 5, 2024 •

edited

Loading