Warnings: remove -Wunused-parameter warnings in Kokkos Kernels #962
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… too To have Kokkos Kernels in line with Kokkos flags we should be able to build without unused-parameter warnings. These changes are making are making this possible using clang-12. Maybe more testing on other platform should be performed with this warning turned on.
This was confusing Github syntax highlighting
e10harvey
approved these changes
May 6, 2021
| @@ -68,7 +68,7 @@ namespace KokkosBlas { | |||
| void SerialTrmm_Invoke (const char side[], | |||
| const char uplo[], | |||
| const char trans[], | |||
| const char diag[], | |||
| const char /*diag*/[], | |||
Comment on lines
308
to
309
| std::ostream &out, | ||
| //Teuchos::FancyOStream& out, |
Comment on lines
389
to
390
| std::ostream &/*out*/, | ||
| //Teuchos::FancyOStream& /* out */, |
@lucbv, I think they can be added to the GCC flags in cm_test_all. |
brian-kelley
requested changes
May 6, 2021
| @@ -249,20 +253,24 @@ void __do_trmm_serial_blas(options_t options, trmm_args_t trmm_args) { | |||
| Kokkos::fence(); | |||
| } | |||
| __trmm_output_csv_row(options, trmm_args, timer.seconds()); | |||
| return; | |||
There was a problem hiding this comment.
A lot of these blas3 perf tests have return statements at the end of void functions - are they necessary?
There was a problem hiding this comment.
Actually I believe that both return; and return void(); are valid in a void function, it's just a stylistic preference so I don't really see the need to remove these either?
If you feel strongly we could add a guideline on that for developers (I actually don't feel strongly anyway) :p
The only one that is actually needed and can be void is this one:
template <class T>
T fun() {
return T();
}See stackoverflow
There was a problem hiding this comment.
Oh sure, it's valid, I was just thinking that we could remove a few lines of code. It's definitely not a big deal though.
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
@lucbv There's probably a lot of really old stuff in the Utils files like the example you had (kk_get_lower_triangle_fill_parallel and kk_get_lower_triangle_fill*) which are not used anywhere. I think we could deprecate a lot of that. |
I just ran clang-format on the badly formatted files. I removed function arguments from `main(int, argv**)` as they are really not justified and also added space between pointers and comments to make github happy.
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Console Output (last 100 lines) : KokkosKernels_PullRequest_GCC720_Light # 219 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA10 # 183 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA9 # 180 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_GCC720_GCC740 # 178 (click to expand) |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
Updating the list of warnings for each compilers in the test scripts. This should trigger the warnings in the auto-tester. This has been tested on Blake manually already but this change will trigger the testing on White and Kokkos-dev2 automatically.
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Console Output (last 100 lines) : KokkosKernels_PullRequest_GCC720_Light # 223 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA10 # 187 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA9 # 184 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_GCC720_GCC740 # 182 (click to expand) |
This finishes to fix the warnings related to unused parameters that were in CUDA specific code branches.
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Console Output (last 100 lines) : KokkosKernels_PullRequest_GCC720_Light # 230 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_GCC720_Light_LayoutRight # 8 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA10 # 194 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA10_LayoutRight # 7 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_CUDA9 # 191 (click to expand)Console Output (last 100 lines) : KokkosKernels_PullRequest_Tpls_GCC720_GCC740 # 189 (click to expand) |
Small update as parameter is commented it should not be refered to in the non-CUDA path anymore...
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720_GCC740
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@brian-kelley if you have a minute to look at this again, I think we can probably get this merged? |
brian-kelley
approved these changes
May 10, 2021
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ brian-kelley ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To have Kokkos Kernels in line with Kokkos flags we should be able to build without unused-parameter warnings.
I am somewhat surprised that these flags from Kokkos are not picked up by our builds?
This PR is fairly large but actually has very minor impact on the code base as it primarily comments unused function parameters... but it also reveals a few strange and worrisome patterns like not setting/checking test results.
Also I spotted a bug where an
intis declared as abool, see below:Maybe more testing on other platform should be performed with this warning turned on.
If you have any comments as to how things should be done let me know.
With these changes I can build with
-Werror -Wunused-parameteron tulip with clang-12, I will setup a build that reproduces the auto-tester config using these flags to see if we could set these on in CI testing.