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.

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

Jump to bottom

[GPU] Conv should not extend strides and dilations for 3d output #26415

Merged
merged 2 commits into from
Oct 10, 2024
Merged

[GPU] Conv should not extend strides and dilations for 3d output #26415

merged 2 commits into from
Oct 10, 2024

Conversation

kelvinchoi-intel
Copy link
Contributor

@kelvinchoi-intel kelvinchoi-intel commented Sep 4, 2024 •
edited
Loading

Details:

  • Conv should not extend strides and dilations for 3d output
  • Update not to call get_dims() for dynamic shape

Tickets:

  • 150680

@github-actions github-actions bot added the category: GPU OpenVINO GPU plugin label Sep 4, 2024
@kelvinchoi-intel kelvinchoi-intel force-pushed the enable_melo_tts branch 3 times, most recently from 03c60d0 to aec80e9 Compare September 9, 2024 01:14
isanghao
isanghao reviewed Sep 23, 2024
View reviewed changes
src/plugins/intel_gpu/src/plugin/ops/convolution.cpp Outdated
@@ -44,7 +44,7 @@ static void CreateConvolutionOp(ProgramBuilder& p, const std::shared_ptr<ov::int
auto pads_end = op->get_pads_end();
auto auto_pad = op->get_auto_pad();

if (!op->is_dynamic()) {
if (!op->is_dynamic() && outDims.size() >= 4) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain more about this code? The commit message said "Conv doens't extend strides and dilations for 3d output". Does it mean that "conv should not extend ..."?
I think this code itself is about to handle 3d case properly because 1d vector is used when outDims.size() == 3. Is there a case where this condition is true?

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 it means, "conv should not extend ...".
  • 1d conv has 3d input/output shape. The extension code looks like old style to handling minimum 4d tensor.
  • "Is there a case where this condition is true?" --> stride is 1d at original model, but input/output becomes 4d case at original or during optimization possibly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then please update commit message accordingly. Could you check whether this code is executed? I think you can check it with the broader set of models that has 1d convolution. I think we need to remove this block if it is of no use. Also, please run jenkins testing.

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've update the condition with !p.use_new_shape_infer(). We may delete whole 'if' code block after old shpae infer is no more used.

isanghao
isanghao reviewed Sep 23, 2024
View reviewed changes
...plugins/intel_gpu/tests/functional/shared_tests_instances/single_layer_tests/convolution.cpp
::testing::Values(ov::test::static_partial_shapes_to_test_representation(std::vector<ov::PartialShape>({{1, 256, 1}}))),
::testing::Values(ov::test::utils::DEVICE_GPU)),
ConvolutionLayerTest::getTestCaseName);

Copy link
Contributor

Choose a reason for hiding this comment

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

does this test fail without fixing the code? I tried this and it succeeded without your fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This 1D conv test is just for filling the 1d conv unit test to ensure 1d case because of it's missing part. This test is not related to the fix code.
"smoke_static_conv_n_dynamic_concat" functional test is related with the fix code.

@kelvinchoi-intel kelvinchoi-intel changed the title [GPU] Conv doens't extend strides and dilations for 3d output [GPU] Conv should not extend strides and dilations for 3d output Sep 24, 2024
@isanghao isanghao added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2024
@p-durandin p-durandin added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 7, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 7, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 7, 2024
@kelvinchoi-intel
[GPU] Conv should not extend strides and dilations for 3d output (#26415
e55cff7 
)

### Details:
 - *Conv should not extend strides and dilations for 3d output*
 - *Update not to call get_dims() for dynamic shape*

### Tickets:
 - *150680*
@kelvinchoi-intel kelvinchoi-intel removed this pull request from the merge queue due to a manual request Oct 7, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 8, 2024
@kelvinchoi-intel kelvinchoi-intel added this pull request to the merge queue Oct 10, 2024
Merged via the queue into openvinotoolkit:master with commit ebeee18 Oct 10, 2024
142 checks passed
@kelvinchoi-intel kelvinchoi-intel deleted the enable_melo_tts branch October 10, 2024 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isanghao isanghao isanghao approved these changes

Assignees
No one assigned
Labels
category: GPU OpenVINO GPU plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kelvinchoi-intel @isanghao