Skip to content

[SYCL] Fix various compilation warnings in plugins #2979

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
Merged
merged 5 commits into from
Jan 16, 2021
Merged

[SYCL] Fix various compilation warnings in plugins #2979

merged 5 commits into from
Jan 16, 2021

Conversation

sergey-semenov
Copy link
Contributor

  • default label in switch which covers all enumeration items
  • unused variables

@sergey-semenov
[SYCL] Fix various compilation warnings in plugins
2f7bff8 
- default label in switch which covers all enumeration items
- unused variables
smaslov-intel
smaslov-intel reviewed Jan 4, 2021
View reviewed changes
smaslov-intel
smaslov-intel previously approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM, but needs rework if those "redundant" "default" warnings re-appear.

smaslov-intel
smaslov-intel reviewed Jan 6, 2021
View reviewed changes
@sergey-semenov sergey-semenov requested a review from a team as a code owner January 11, 2021 22:42
@cperkinsintel
Copy link
Contributor

default: for switches restored, and covered-switch-default flag disabled for SYCL library builds (only).

@cperkinsintel
Copy link
Contributor

Hmm - I have re-run the Jenkins PreCommit, and it is passing: http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FLin%2FLLVM_Test_Suite/detail/LLVM_Test_Suite/1306/pipeline

But the status here on GitHub seems to not be updated.

@cperkinsintel cperkinsintel marked this pull request as draft January 13, 2021 23:52
@cperkinsintel cperkinsintel marked this pull request as ready for review January 13, 2021 23:53
@cperkinsintel
force CI to re-run
69415b4 
Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel
Copy link
Contributor

ping to reviewers

@v-klochkov
Copy link
Contributor

ping to reviewers

Your change adds -Wno-covered-switch-default for SYCL.

I would rather vote for the opposite, i.e. for enforcing -Wswitch together with -Wcovered-switch-default.
Sure, there may be cons to that approach such as cases casts from int to enum, but IMO that is not good practice, and such cases could be treated better.

Please, do not consider my comment as a blocker. I just would like to let other reviewers from @llvm-reviewers-runtime review/decide.

@v-klochkov v-klochkov removed their request for review January 14, 2021 21:54
@cperkinsintel
Copy link
Contributor

@v-klochkov

Sure, there may be cons to that approach such as cases casts from int to enum, but IMO that is not good practice, and such cases could be treated better.

Better than just using default: ? It's succinct, and in our usage it makes the switches over enums correctly return PI_INVALID_VALUE . If we remove those defaults, the code gets uglier and harder to read. I guess there is a small benefit in that if we don't use the default clause then if the enum changes in the plugin interface, the -Wswitch flag will immediately alert us to those plugins that need updating. But my own inclination is that this will likely be performed at the time of the update to the enumeration and can also be caught by testing.

Ultimately, I have no stake. But this PR has already been revised several times as we've chased this tail behind the dog. I'll be happy if we just take a position.

smaslov-intel
smaslov-intel reviewed Jan 15, 2021
View reviewed changes
smaslov-intel
smaslov-intel previously approved these changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

Thanks!

smaslov-intel
smaslov-intel approved these changes Jan 16, 2021
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

There isn't any change since my last review, right?

@v-klochkov v-klochkov merged commit 9848d9f into intel:sycl Jan 16, 2021
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jan 18, 2021
Merge branch 'sycl' into clang_warnings
06d0a57 
* sycl:
  [sycl-post-link][NFC] Extracted the code into a subroutine (intel#3042)
  [SYCL][NFC] Remove commented out code (intel#3029)
  [CODEOWNERS] Fix ownership of DPC++ tools tests (intel#3047)
  [SYCL][NFC] Make tests insensitive to dso_local (intel#3037)
  [SYCL] Fix acquiring a mutex in _pi_context::finalize (intel#3001)
  [SYCL] Fix various compilation warnings in plugins (intel#2979)
@sergey-semenov sergey-semenov deleted the fixpluginwarnings branch January 18, 2021 10:48
@sergey-semenov sergey-semenov restored the fixpluginwarnings branch January 18, 2021 10:49
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Jan 19, 2021
Merge branch 'sycl' into clang_gh_actions
d0f22f3 
* sycl: (378 commits)
  [sycl-post-link][NFC] Extracted the code into a subroutine (intel#3042)
  [SYCL][NFC] Remove commented out code (intel#3029)
  [CODEOWNERS] Fix ownership of DPC++ tools tests (intel#3047)
  [SYCL][NFC] Make tests insensitive to dso_local (intel#3037)
  [SYCL] Fix acquiring a mutex in _pi_context::finalize (intel#3001)
  [SYCL] Fix various compilation warnings in plugins (intel#2979)
  [SYCL][ESIMD] Add simd class conversion ctor and operator (intel#3028)
  [sycl-post-link][NFC] Use range-based for loop. (intel#3033)
  [SYCL][NFC] Fix warning in self-build (intel#3023)
  [NFC] Fix sycl-post-link tests to avoid potential race (intel#3031)
  [SYCL][CUDA] Add missing barrier to collectives (intel#2990)
  [SYCL] Make Intel attributes consistent with clang attributes. (intel#3022)
  [SYCL] Bump SYCL minor version (intel#3026)
  [SYCL][Doc] Added requirement on reference to test PR in commit message (intel#3010)
  [SYCL] Put constant initializer list data in non-generic addr space. (intel#3005)
  [SYCL][L0] Fix memory leak in PiDeviceCache and ZeCommandList (intel#2974)
  [SYCL] Fix detection of free function calls (intel#3003)
  [SYCL][NFC] Clean up the builder_dir argument description (intel#3021)
  [SYCL][ESIMD] Fix LowerESIMD crash on a scalar fptoui LLVM instruction (intel#2699)
  [NFC] Remove redundant call to getMainExecutable() (intel#3018)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cperkinsintel cperkinsintel cperkinsintel left review comments

@v-klochkov v-klochkov v-klochkov left review comments

@smaslov-intel smaslov-intel smaslov-intel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sergey-semenov @cperkinsintel @v-klochkov @smaslov-intel