Remove unused variables #2044
Remove unused variables #2044
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes unused variables from the codebase and adds a compiler flag to detect unused variables in the future. The changes focus on cleaning up variables that were declared but never referenced in the code.
- Removes unused
output_sizevariable from RoiAlign kernel - Removes unused
dg_dataanddb_datavariables from LayerNorm backward kernel - Adds
-Werror=unused-variablecompiler flag to catch future unused variables
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ATen/native/xpu/sycl/RoiAlignKernels.cpp | Removes unused output_size variable calculation |
| src/ATen/native/xpu/sycl/LayerNormKernels.cpp | Removes unused pointer variables dg_data and db_data |
| CMakeLists.txt | Adds compiler flag to treat unused variables as errors |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CMakeLists.txt
Outdated
| @@ -25,6 +25,7 @@ set(PROJECT_VERSION "2.3.0") | |||
| # Avoid SYCL compiler error | |||
| if(NOT WIN32) | |||
| string(APPEND CMAKE_CXX_FLAGS " -Wno-error") | |||
| string(APPEND CMAKE_CXX_FLAGS " -Werror=unused-variable") | |||
There was a problem hiding this comment.
Enabling this option is good for the final code quality, but it also requires every compilation in development to follow this rule. Such checks would be better placed in CI.
There was a problem hiding this comment.
Ok, I've passed this flag as env variable in linux_build CI pipeline
.github/workflows/_linux_build.yml
Outdated
| @@ -94,6 +94,7 @@ jobs: | |||
| - name: Build Pytorch on ${{ needs.runner.outputs.hostname }} | |||
| run: | | |||
| export USE_XCCL=1 | |||
| export CXXFLAGS="${CXXFLAGS} -Werror=unused-variable" | |||
There was a problem hiding this comment.
Exporting this flag would propagate it to the entire PyTorch build, which may cause build failures in third-party components.
There was a problem hiding this comment.
I'd opt for keeping this in CMake, because even if it affects each dev-build, it's beneficial to get rid of unused code as soon as possible. Other possibility is to have some is CI flag, so it would be executed only for CI builds
There was a problem hiding this comment.
@CuiYifeng, I think we should always follow the rule during daily development, as I don't quite understand the logic behind unused variables. What's the motivation in your mind to bypass the logic for regular development?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -25,6 +25,9 @@ set(PROJECT_VERSION "2.3.0") | |||
| # Avoid SYCL compiler error | |||
| if(NOT WIN32) | |||
| string(APPEND CMAKE_CXX_FLAGS " -Wno-error") | |||
| if("$ENV{IS_XPU_CI}" STREQUAL "1") | |||
| string(APPEND CMAKE_CXX_FLAGS " -Werror=unused-variable") | |||
There was a problem hiding this comment.
Should we always turn on this build option?
Uh oh!
There was an error while loading. Please reload this page.