Skip to content

Conversation

@inocsin
Copy link
Contributor

@inocsin inocsin commented Feb 5, 2021

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
    Fix aten::adaptive_avg_pool2d bug when input size is not integer multiple of output size, update the test case
  • New feature (non-breaking change which adds functionality)
    Support aten::adaptive_max_pool2d with test case
    -Bug fix
    Remove "#if NV_TENSORRT_MAJOR < 7 || (NV_TENSORRT_MAJOR == 7 && NV_TENSORRT_MINOR < 1)" part.
    Since the TensorRT dependency is updated, the original GPU part will never be used, all the operator will be forced to run on cpu. So I remove the TensorRT flag. But the GPU part failed with unknow reason after the flag was removed, so I refactored the gpu part, now it can run on GPU correctly.

Checklist:

  • My code follows the style guidelines of this project (You can use the linters)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas and hacks
  • I have made corresponding changes to the documentation
  • I have added tests to verify my fix or my feature
  • New and existing unit tests pass locally with my changes

@github-actions github-actions bot added component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: core Issues re: The core compiler component: tests Issues re: Tests labels Feb 5, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

There are some changes that do not conform to C++ style guidelines:

diff --git a/workspace/core/conversion/converters/impl/plugins/adaptive_max_pool2d_plugin.cpp b/tmp/changes.txt
index 84a2e1f..597c30e 100644
--- a/workspace/core/conversion/converters/impl/plugins/adaptive_max_pool2d_plugin.cpp
+++ b/tmp/changes.txt
@@ -194,10 +194,8 @@ int AdaptiveMaxPool2dPlugin::enqueue(
    cudaStream_t stream) {
#if NV_TENSORRT_MAJOR < 7 || (NV_TENSORRT_MAJOR == 7 && NV_TENSORRT_MINOR < 1)
  at::Tensor input = at::from_blob((void*)inputs[0], util::toVec(inputDesc->dims), [](void*) {}, tensor_options_);
-  at::Tensor output = at::from_blob(
-      outputs[0], util::volume(outputDesc->dims), [](void*) {}, tensor_options_);
-  at::Tensor indices = at::from_blob(
-      outputs[1], util::volume(outputDesc->dims), [](void*) {}, index_tensor_options_);
+  at::Tensor output = at::from_blob(outputs[0], util::volume(outputDesc->dims), [](void*) {}, tensor_options_);
+  at::Tensor indices = at::from_blob(outputs[1], util::volume(outputDesc->dims), [](void*) {}, index_tensor_options_);

  at::cuda::CUDAStream torch_stream = at::cuda::getStreamFromPool();
  at::cuda::CUDAStreamGuard torch_guard(torch_stream);
diff --git a/workspace/core/conversion/converters/impl/plugins/interpolate_plugin.cpp b/tmp/changes.txt
index ff9ba86..5689d88 100644
--- a/workspace/core/conversion/converters/impl/plugins/interpolate_plugin.cpp
+++ b/tmp/changes.txt
@@ -254,8 +254,7 @@ int InterpolatePlugin::enqueue(
    cudaStream_t stream) {
#if NV_TENSORRT_MAJOR < 7 || (NV_TENSORRT_MAJOR == 7 && NV_TENSORRT_MINOR < 1)
  at::Tensor input = at::from_blob((void*)inputs[0], util::toVec(inputDesc->dims), [](void*) {}, tensor_options_);
-  at::Tensor output = at::from_blob(
-      outputs[0], util::volume(outputDesc->dims), [](void*) {}, tensor_options_);
+  at::Tensor output = at::from_blob(outputs[0], util::volume(outputDesc->dims), [](void*) {}, tensor_options_);

  at::cuda::CUDAStream torch_stream = at::cuda::getStreamFromPool();
  at::cuda::CUDAStreamGuard torch_guard(torch_stream);
ERROR: Some files do not conform to style guidelines

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code conforms to Python style guidelines

@narendasan
Copy link
Collaborator

Can you apply the linting?

Signed-off-by: inocsin <[email protected]>
@inocsin
Copy link
Contributor Author

inocsin commented Feb 10, 2021

Can you apply the linting?

applied linting

@peri044
Copy link
Collaborator

peri044 commented Feb 22, 2021

The tests are running fine. It looks like the existing Interpolate plugin already calls at::adaptive_avg_pool2d by using the mode adaptive_pool2d https://github.com/NVIDIA/TRTorch/blob/master/core/conversion/converters/impl/plugins/interpolate_plugin.cpp#L329
Can we make use of the mode flag to support adaptive_max_pool2d as well ? Does this require a separate plugin in this PR?

@inocsin
Copy link
Contributor Author

inocsin commented Feb 24, 2021

The tests are running fine. It looks like the existing Interpolate plugin already calls at::adaptive_avg_pool2d by using the mode adaptive_pool2d https://github.com/NVIDIA/TRTorch/blob/master/core/conversion/converters/impl/plugins/interpolate_plugin.cpp#L329
Can we make use of the mode flag to support adaptive_max_pool2d as well ? Does this require a separate plugin in this PR?

The output number, datatype and parameter number of adaptive_max_pool are different from interpolate_plugin, in that case should we use several if-else to merge these two plugin into one?

@narendasan narendasan added the on hold PR is on hold pending another PR or feature being implemented label Mar 19, 2021
inocsin added 2 commits March 23, 2021 18:38
…n cannot use gpu to accelerate

Signed-off-by: inocsin <[email protected]>
Signed-off-by: inocsin <[email protected]>
@inocsin
Copy link
Contributor Author

inocsin commented Mar 23, 2021

@narendasan @peri044 please review the update, I have fixed some bugs of original interpolate plugin

@peri044
Copy link
Collaborator

peri044 commented May 18, 2021

Thanks @inocsin Merged these changes into the new plugins interface. Tried to cherry pick your commits but there are lot of changes in the new API which were limiting it.

@peri044 peri044 closed this May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: core Issues re: The core compiler component: plugins component: tests Issues re: Tests on hold PR is on hold pending another PR or feature being implemented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants