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

[TF Quant Models] Prepares GroupConvolution weights #25641

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

jane-intel
Copy link
Contributor

Details:

  • using register_new_node in the matches to allow for seamless matcher application for the FQ node
  • added optional Convert node, so that pattern would work on different quantized weights forms

image

Tickets:

@jane-intel jane-intel requested a review from a team as a code owner July 19, 2024 12:20
@jane-intel jane-intel requested review from itikhono and removed request for a team July 19, 2024 12:20
@github-actions github-actions bot added the category: transformations OpenVINO Runtime library - Transformations label Jul 19, 2024
Comment on lines +72 to +78
auto reshaped_input = reshape_node->clone_with_new_inputs(
{limit_input,
ov::op::v0::Constant::create(element::i64, {new_limit_shape.size()}, new_limit_shape)}));
ov::op::v0::Constant::create(element::i64, {new_limit_shape.size()}, new_limit_shape)});
if (auto constant = ov::util::get_constant_from_source(reshaped_input)) {
reshaped_input = constant;
}
renewed_inputs.push_back(reshaped_input);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: can we use ov::op::util::clone_try_fold instead to make the code a bit shorter? The same at L87 and at L60 in pull_transpose_through_fq.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that, sure. The only difference is that clone_try_fold only works for single node folding and get_constant_from_source checks all the necessary no_folding rt info and could fold a chain of nodes. For now, I won't change the code in the PR, however, in case more feedback arises -- I'll clean it up.

if (pattern_map.count(convert_p)) {
const auto& convert_node = pattern_map.at(convert_p).get_node_shared_ptr();
convert_node->input(0).replace_source_output(reshaped_input);
convert_node->validate_and_infer_types();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just wondering: why should we manually call node validation here? It is usually automatically done by Validate pass, isn't it?

Upd. It seems like it is needed to keep a valid graph state during one GraphRewrite pass. Am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you ARE right. Since we are in a single Graph Rewrite -- it is safer to keep the graph in validated state, so that all the matchers could assume rank and shape is accurate

@jane-intel jane-intel requested a review from v-Golubev July 23, 2024 12:19
@jane-intel
Copy link
Contributor Author

Performance validation is triggered for this particular PR with IR cache generation and calibration. No code is expected to be pushed in the branch. Merging is postponed until PR is proven to be non-regressive.

Copy link
Contributor

github-actions bot commented Sep 1, 2024

This PR will be closed in a week because of 2 weeks of no activity.

@github-actions github-actions bot added the Stale label Sep 1, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 2 week with no activity.

@github-actions github-actions bot closed this Sep 10, 2024
@jane-intel jane-intel reopened this Sep 12, 2024
@github-actions github-actions bot removed the Stale label Sep 13, 2024
Copy link
Contributor

github-actions bot commented Oct 5, 2024

This PR will be closed in a week because of 2 weeks of no activity.

@github-actions github-actions bot added the Stale label Oct 5, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 2 week with no activity.

@github-actions github-actions bot closed this Oct 13, 2024
@jane-intel jane-intel reopened this Nov 11, 2024
@jane-intel
Copy link
Contributor Author

I could not get the results for this PR. I propose merging the change to master and seeing if regressions arise in the regular validation.

@jane-intel jane-intel added this pull request to the merge queue Nov 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 11, 2024
@github-actions github-actions bot removed the Stale label Nov 12, 2024
@jane-intel jane-intel added this pull request to the merge queue Nov 12, 2024
Merged via the queue into openvinotoolkit:master with commit ff4d1e5 Nov 12, 2024
165 checks passed
@jane-intel jane-intel deleted the CVS-39818 branch November 12, 2024 08:05
NishantPrabhuFujitsu pushed a commit to NishantPrabhuFujitsu/openvino that referenced this pull request Nov 26, 2024
…25641)

### Details:
- *using register_new_node in the matches to allow for seamless matcher
application for the FQ node*
- *added optional Convert node, so that pattern would work on different
quantized weights forms*


![image](https://github.com/user-attachments/assets/2d485f5b-7609-40ac-b798-16124a9a290a)

### Tickets:
 - *CVS-39818*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants