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

[LPT] Quantized LSTMSequence & GRUSequence extended support #25654

Merged

Conversation

eshoguli
Copy link
Contributor

@eshoguli eshoguli commented Jul 19, 2024

Details:

  • Low Precision Transformations: Quantized LSTMSequence & GRUSequence extended support

Tickets:

@eshoguli eshoguli requested a review from a team as a code owner July 19, 2024 20:08
@github-actions github-actions bot added the category: LP transformations OpenVINO Low Precision transformations label Jul 19, 2024
@eshoguli eshoguli added the category: CPU OpenVINO CPU plugin label Jul 20, 2024
@eshoguli eshoguli closed this Jul 22, 2024
@eshoguli eshoguli deleted the es/lpt/lstm_support_extension branch July 22, 2024 10:39
@eshoguli eshoguli restored the es/lpt/lstm_support_extension branch July 22, 2024 10:48
@eshoguli eshoguli reopened this Jul 22, 2024
@github-actions github-actions bot removed the category: CPU OpenVINO CPU plugin label Jul 22, 2024
@eshoguli eshoguli force-pushed the es/lpt/lstm_support_extension branch from 717d7bd to e7840ff Compare July 22, 2024 18:44
@eshoguli eshoguli requested review from a team as code owners July 22, 2024 18:44
@github-actions github-actions bot added category: IE Tests OpenVINO Test: plugins and common category: CPU OpenVINO CPU plugin labels Jul 22, 2024
@eshoguli eshoguli requested review from a team as code owners July 22, 2024 23:03
@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Jul 22, 2024
@eshoguli eshoguli force-pushed the es/lpt/lstm_support_extension branch 5 times, most recently from 13709dc to 9708278 Compare July 24, 2024 09:54
Copy link
Contributor

@v-Golubev v-Golubev left a comment

Choose a reason for hiding this comment

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

Please remove debug info (serialization, couts) and fix the CI

@eshoguli eshoguli requested a review from v-Golubev July 24, 2024 23:22
@eshoguli eshoguli force-pushed the es/lpt/lstm_support_extension branch from 4487204 to 715e207 Compare July 24, 2024 23:39
Copy link
Contributor

@v-Golubev v-Golubev left a comment

Choose a reason for hiding this comment

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

I am still concerned by the current implementation. We can merge it, but I think we need to prioritize CVS-147588 to remove this WA as soon as possible


BroadcastTransformation::BroadcastTransformation(const Params& params) : TransparentBaseTransformation(params) {
MATCHER_SCOPE(BroadcastTransformation);
auto matcher = pattern::wrap_type<ov::opset1::Broadcast>({
Copy link
Contributor

Choose a reason for hiding this comment

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

We still don't match on v3::Broadcast here. Maybe we can match on BroadcastBase?

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 need specify supported operation here, not base types, which can be inheritted by not supported operation in future.


std::vector<std::pair<size_t, element::Type>> get_quantized_inputs(std::shared_ptr<ov::Node> lstm) {
return is_type<ov::opset5::LSTMSequence>(lstm) ?
std::vector<std::pair<size_t, element::Type>>{ {0, element::u8}, { 1, element::u8 }, { 4, element::undefined }, { 5, element::undefined } } :
Copy link
Contributor

Choose a reason for hiding this comment

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

We always regulate the desired precisions on inputs with PrecisionsRestriction which are set by plugin. In CPU, we actually have u8u8 restrictions:

            PrecisionsRestriction::create<ov::opset5::LSTMSequence>({
                    {{0, 1}, {ov::element::u8}}
                }),
            PrecisionsRestriction::create<ov::opset6::GRUSequence>({
                    {{0, 1}, {ov::element::u8}}
                }),

Why do we need this additional logic with precisions limitations here? If there is no chance to avoid it, could you please add an explanatory comment in the code on what undefined precision means?

Copy link
Contributor Author

@eshoguli eshoguli Jul 25, 2024

Choose a reason for hiding this comment

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

We need it here.
LPT uses operation input precision restrictions to identify output precision for FakeQuantize dequantization transformation. But it doesn't mean, that we can ignore filter or input validation in each transformation pattern matching, because in Constant branch (for example, a branch with single Constant operation) LPT can not provide require precision.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a valid point, but it seems like we have such checks only in this transformation. Does it mean that we must perform the checks in each transformation (or at least in the transformations which work with layers with weights)?

@eshoguli eshoguli force-pushed the es/lpt/lstm_support_extension branch from a29d204 to dd6f642 Compare July 25, 2024 18:29
@eshoguli eshoguli requested a review from v-Golubev July 25, 2024 18:30
@eshoguli eshoguli requested a review from v-Golubev July 26, 2024 12:25
@eshoguli eshoguli added this pull request to the merge queue Jul 27, 2024
Merged via the queue into openvinotoolkit:master with commit 3056b53 Jul 27, 2024
124 checks passed
@eshoguli eshoguli deleted the es/lpt/lstm_support_extension branch July 27, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: CPU OpenVINO CPU plugin category: GPU OpenVINO GPU plugin category: IE Tests OpenVINO Test: plugins and common category: LP transformations OpenVINO Low Precision transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants