Skip to content
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.

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

[luci/pass] Revise FuseInstanceNormPass V6 #14397

Closed
wants to merge 1 commit into from

Conversation

seanshpark
Copy link
Contributor

This will revise FuseInstanceNormPass version_6 to have const_as_beta as zero values.

This will revise FuseInstanceNormPass version_6 to have const_as_beta as zero values.

ONE-DCO-1.0-Signed-off-by: SaeHie Park <[email protected]>
@seanshpark
Copy link
Contributor Author

@jinevening , is this what you want ?

Comment on lines +734 to +739
// create 1.0 gamma and 0.0 beta
auto graph = add_as_terminal->graph();
const_as_gamma = make_const_one(graph, 1.0f);
const_as_gamma->name(add_as_terminal->name() + "/gamma");
const_as_beta = make_const_one(graph, 0.0f);
const_as_beta->name(add_as_terminal->name() + "/beta");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these lines are unnecessary. *1 and +0 are Nop.

And, apply functions for version_5 and version_6 have to be updated too. There are no gamma and beta for those patterns, so reshape_gamma_beta function should not be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these lines are unnecessary. *1 and +0 are Nop.

I don't understand the meaning. Maybe code example will help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And, apply functions for version_5 and version_6 have to be updated too.

I cannot check for ver 5 as of now and do not want to mix for both version in one PR.

Comment on lines -727 to -732
const_as_beta = dynamic_cast<luci::CircleConst *>(sub->x());
CHECK_OR_FALSE(const_as_beta);
CHECK_OR_FALSE(const_as_beta->rank() == 3);
CHECK_OR_FALSE(
const_as_beta->dim(0).value() == 1 && const_as_beta->dim(1).value() == input_channel &&
(const_as_beta->dim(2).value() == 1 || const_as_beta->dim(2).value() == input_last_dim));
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to check the shape and the value of sub->x() (const_as_zero).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we need to check all the values should be 0.0f too ?

@seanshpark
Copy link
Contributor Author

@jinevening , I think it would be better you to update V6 with your thoughts.

@seanshpark seanshpark closed this Dec 3, 2024
@seanshpark seanshpark deleted the luci_pass_fusein_v6 branch December 3, 2024 01:44
@jinevening
Copy link
Contributor

Sorry for late reply. I think we should first determine how to support groupnorm in our backend. If it's done, our task may become clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants