Skip to content

[luci] Use static dimensions from new_shape attribute #14844

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.

Sign up for GitHub

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

Closed
mbencer wants to merge 2 commits into from
Closed

[luci] Use static dimensions from new_shape attribute #14844

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4da310
[luci] Use static dimensions from new_shape attribute
mbencer Mar 19, 2025
d3b46c2
review remarks
mbencer Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions compiler/luci/service/src/Nodes/CircleReshape.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@ loco::TensorShape Algorithm::visit(const luci::CircleReshape *node)
// for non-existing `shape`, we can use `newShape` if it's valid
auto new_shape = node->newShape();
auto rank = new_shape->rank();
auto shape_dummy = dynamic_cast<luci::CircleOutputDummy *>(node->shape());
if (shape_dummy && rank > 0)
if (rank > 0)
Copy link
Contributor

@seanshpark seanshpark Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q) why did you remove to check shape_dummy ?

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@seanshpark seanshpark Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what and how CircleOutputDummy works?

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor

@seanshpark seanshpark Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
is_static_shape = true;
shape_by_input.rank(rank);
Expand Down Expand Up @@ -154,7 +153,14 @@ loco::TensorShape Algorithm::visit(const luci::CircleReshape *node)

for (uint32_t axis = 0; axis < shape_by_attr.rank(); ++axis)
{
shape_by_attr.dim(axis) = node->newShape()->dim(axis);
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
}
Comment on lines +156 to +163
Copy link
Contributor

@seanshpark seanshpark Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this change related with removing shape_dummy ?

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@seanshpark seanshpark Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is only to not show info log about shape mismatch.

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

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@seanshpark seanshpark Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@seanshpark seanshpark Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
37 changes: 37 additions & 0 deletions compiler/luci/service/src/Nodes/CircleReshape.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,40 @@ TEST(ShapeRuleTest, reshape_by_newShape)
ASSERT_EQ(2, output_shape.dim(0).value());
ASSERT_EQ(12, output_shape.dim(1).value());
}

TEST(ShapeRuleTest, reshape_by_newShape_dynamic)
{
auto g = loco::make_graph();
auto node_reshape = g->nodes()->create<luci::CircleReshape>();
auto tensor_input = g->nodes()->create<luci::CircleInput>();
auto target_shape = g->nodes()->create<luci::CircleInput>();
;
Copy link
Contributor

@seanshpark seanshpark Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
;

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for catching it. removed


tensor_input->dtype(loco::DataType::S32);
tensor_input->shape({2, 3, 4});
tensor_input->shape_status(luci::ShapeStatus::VALID);

target_shape->dtype(loco::DataType::S32);
target_shape->rank(1);
target_shape->shape_status(luci::ShapeStatus::VALID);

node_reshape->tensor(tensor_input);
node_reshape->shape(target_shape);

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

loco::TensorShape output_shape;
luci::sinf::Rule shape_inf_rule;

ASSERT_TRUE(shape_inf_rule.infer(node_reshape, output_shape));

ASSERT_EQ(3, output_shape.rank());
ASSERT_FALSE(output_shape.dim(0).known());
ASSERT_TRUE(output_shape.dim(1).known());
ASSERT_EQ(4, output_shape.dim(1).value());
ASSERT_FALSE(output_shape.dim(2).known());
}
Copy link
Contributor

@seanshpark seanshpark Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@seanshpark seanshpark Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mbencer mbencer Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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)