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

Fix compiler warnings when compiling with nvc++ #4198

Merged
merged 2 commits into from
Sep 2, 2021
Merged

Fix compiler warnings when compiling with nvc++ #4198

merged 2 commits into from
Sep 2, 2021

Conversation

masterleinad
Copy link
Contributor

Split from #4196. In particular, the half_t operations must use references.

@masterleinad
Copy link
Contributor Author

@e10harvey Could you contribute a test for the (presumably) faulty half_t operations?

@masterleinad masterleinad mentioned this pull request Jul 30, 2021
Merged
dalg24
dalg24 reviewed Aug 2, 2021
View reviewed changes
core/unit_test/TestPolicyConstruction.hpp
@@ -795,7 +795,7 @@ TEST(TEST_CATEGORY, desired_occupancy_empty_base_optimization) {
static_assert(sizeof(decltype(policy)) == 1, "");
static_assert_dummy_policy_must_be_size_one<sizeof(decltype(policy))>
_assert1{};
(void)_assert1; // avoid unused variable warning
(void)&_assert1; // avoid unused variable warning
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvc++ still complained here using (void) but this syntax seems to silence the warning (on all CI checks).

e10harvey
e10harvey reviewed Aug 2, 2021
View reviewed changes
Copy link
Contributor

@e10harvey e10harvey left a comment

Choose a reason for hiding this comment

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

Thanks, @masterleinad ! Which are the faulty half_t operations?

core/src/Cuda/Kokkos_Cuda_Half.hpp Outdated Show resolved Hide resolved
@masterleinad
Copy link
Contributor Author

Thanks, @masterleinad ! Which are the faulty half_t operations?

All the places where I changed auto to auto &, i.e.

  • volatile half_t& operator+=(half_t rhs) volatile
  • volatile half_t& operator-=(half_t rhs) volatile
  • volatile half_t& operator*=(half_t rhs) volatile
  • volatile half_t& operator/=(half_t rhs) volatile

for the non-CUDA case.

@masterleinad
Copy link
Contributor Author

Retest this please.

@e10harvey
Copy link
Contributor

All the places where I changed auto to auto &, i.e.

* `volatile half_t& operator+=(half_t rhs) volatile`

* `volatile half_t& operator-=(half_t rhs) volatile`

* `volatile half_t& operator*=(half_t rhs) volatile`

* `volatile half_t& operator/=(half_t rhs) volatile`

for the non-CUDA case.

Ok, this is done in #4101.

@masterleinad
Copy link
Contributor Author

Retest this please.

1 similar comment
@masterleinad
Copy link
Contributor Author

Retest this please.

@masterleinad masterleinad added this to In progress in Kokkos Release 3.5 via automation Aug 6, 2021
@masterleinad masterleinad added this to Awaiting Feedback in Developer: Daniel Arndt Aug 6, 2021
@masterleinad masterleinad moved this from In progress to Awaiting Feedback in Kokkos Release 3.5 Aug 6, 2021
@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Aug 25, 2021
PhilMiller
PhilMiller reviewed Sep 1, 2021
View reviewed changes
core/src/Cuda/Kokkos_Cuda_Half.hpp Outdated
@@ -468,8 +468,8 @@ class half_t {
// Use non-volatile val_ref to suppress:
// "warning: implicit dereference will not access object of type ‘volatile
// __half’ in statement"
auto val_ref = const_cast<impl_type&>(val);
val_ref = __float2half(__half2float(const_cast<impl_type&>(val)) /
auto& val_ref = const_cast<impl_type&>(val);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was definitely fixed in a better way in #4156

PhilMiller
PhilMiller suggested changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

Please merge or rebase this PR to incorporate the changes from #4156, which I think moot all changes in Kokkos_Cuda_Half.hpp - the changes to it here are papering over incorrect code that the compiler legitimately warns about.

@PhilMiller
Copy link
Contributor

To be clear, I'm +1 on the rest of the changes here

@masterleinad
Copy link
Contributor Author

@PhilMiller I rebased.

dalg24
dalg24 reviewed Sep 1, 2021
View reviewed changes
containers/src/Kokkos_DualView.hpp
inline void modify_host() {
if (impl_dualview_is_single_device::value) return;
Copy link
Member

Choose a reason for hiding this comment

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

Would you remind me what warnings we were getting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like

"/tmp/kokkos/containers/unit_tests/../src/Kokkos_DualView.hpp", line 818: warning: statement is unreachable

@crtrott crtrott merged commit b474729 into kokkos:develop Sep 2, 2021
Kokkos Release 3.5 automation moved this from Awaiting Feedback to Done Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@PhilMiller PhilMiller PhilMiller approved these changes

@crtrott crtrott crtrott approved these changes

@e10harvey e10harvey Awaiting requested review from e10harvey

Assignees
No one assigned
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
Developer: Daniel Arndt
Awaiting Feedback
Kokkos Release 3.5
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@masterleinad @e10harvey @PhilMiller @dalg24 @crtrott