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

Checked narrowing conversion in MDRangePolicy construction #3527

Merged
merged 7 commits into from
Oct 29, 2020
Merged

Checked narrowing conversion in MDRangePolicy construction #3527

merged 7 commits into from
Oct 29, 2020

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Oct 26, 2020

Fix #3524

dalg24
dalg24 commented Oct 26, 2020
View reviewed changes
core/src/KokkosExp_MDRangePolicy.hpp
@@ -162,7 +179,7 @@ struct MDRangePolicy : public Kokkos::Impl::PolicyTraits<Properties...> {
enum { rank = static_cast<int>(iteration_pattern::rank) };

using index_type = typename traits::index_type;
using array_index_type = long;
using array_index_type = std::int64_t;
using point_type = Kokkos::Array<array_index_type, rank>; // was index_type
Copy link
Member Author

Choose a reason for hiding this comment

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

You may ask why didn't I change this to

Suggested change
using point_type = Kokkos::Array<array_index_type, rank>; // was index_type
using point_type = Kokkos::Array<index_type, rank>; // was index_type

The answer is because it would potentially break user code :/

dalg24
dalg24 commented Oct 26, 2020
View reviewed changes
core/src/KokkosExp_MDRangePolicy.hpp
@@ -162,7 +179,7 @@ struct MDRangePolicy : public Kokkos::Impl::PolicyTraits<Properties...> {
enum { rank = static_cast<int>(iteration_pattern::rank) };

using index_type = typename traits::index_type;
using array_index_type = long;
using array_index_type = std::int64_t;
Copy link
Member Author

Choose a reason for hiding this comment

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

This change was suggested by @crtrott
It does not absolutely have to be part of this PR.

Copy link
Member Author

@dalg24 dalg24 Nov 13, 2020 •
edited

Choose a reason for hiding this comment

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

Cabana was building MDRangePolicy with arrays of long without setting the index type on the policy.
Although this was invalid use we must recognize this change may break code downstream.

@DavidPoliakoff
Copy link
Contributor

Any idea about the OpenMPTarget failure? I want to dig in on that before approving, though this looks good, I'm not sure what the bug is

@dalg24
Copy link
Member Author

dalg24 commented Oct 27, 2020

Any idea about the OpenMPTarget failure? I want to dig in on that before approving, though this looks good, I'm not sure what the bug is

Because the error message is not detected. I wonder if it has anything to do with it being printed to the standard error.

kokkos/core/src/OpenMPTarget/Kokkos_OpenMPTarget_Abort.hpp

Lines 54 to 57 in 3243f20

KOKKOS_INLINE_FUNCTION void OpenMPTarget_abort(char const *msg) {
fprintf(stderr, "%s.\n", msg);
std::abort();
}

@dalg24
Copy link
Member Author

dalg24 commented Oct 27, 2020

Well abort(msg) is a no-op on OpenMPTarget backend...

kokkos/core/src/impl/Kokkos_Error.hpp

Lines 192 to 202 in 3243f20

KOKKOS_IMPL_ABORT_NORETURN KOKKOS_INLINE_FUNCTION void abort(
const char *const message) {
#if defined(KOKKOS_ENABLE_CUDA) && defined(__CUDA_ARCH__)
Kokkos::Impl::cuda_abort(message);
#elif defined(KOKKOS_ENABLE_HIP) && defined(__HIP_DEVICE_COMPILE__)
Kokkos::Impl::hip_abort(message);
#elif !defined(KOKKOS_ENABLE_OPENMPTARGET) && !defined(__SYCL_DEVICE_ONLY__)
Kokkos::Impl::host_abort(message);
#else
(void)message; // FIXME_OPENMPTARGET, FIXME_SYCL
#endif

@dalg24 dalg24 force-pushed the checked_narrowing_conversion branch from e7ec914 to 569d93a Compare October 27, 2020 16:39
masterleinad
masterleinad reviewed Oct 27, 2020
View reviewed changes
core/src/KokkosExp_MDRangePolicy.hpp Outdated Show resolved Hide resolved
core/src/KokkosExp_MDRangePolicy.hpp
(std::is_signed<To>::value != std::is_signed<From>::value);
auto const ret = static_cast<To>(arg);
if (static_cast<From>(ret) != arg ||
(is_different_signedness && (arg < From{}) != (ret < To{}))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be enough to check that both of them are positive, shouldn't it? If one of the types is unsigned, values of that type cannot be negative anyway.

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Abort.hpp Outdated Show resolved Hide resolved
nliber
nliber reviewed Oct 27, 2020
View reviewed changes
core/src/KokkosExp_MDRangePolicy.hpp Show resolved Hide resolved
core/src/KokkosExp_MDRangePolicy.hpp
// NOTE prefer C array U[M] to std::initalizer_list<U> so that the number of
// elements can be deduced (https://stackoverflow.com/q/40241370)
template <class Array, class U, std::size_t M>
// NOTE for some unfortunate reason the policy bounds are stored as signed
Copy link
Contributor

Choose a reason for hiding this comment

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

This note needs to be expanded upon. The problem isn't just signed vs. unsigned numbers, it is really because there are possibly three separate element types involved, one of which may have a different sign than the others: IndexType, Array::value_type and U.

Even after reading the note, it was not obvious why IndexType is here at all, or why we cannot convert to an array of IndexType here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dalg24 dalg24 force-pushed the checked_narrowing_conversion branch from 569d93a to b212d46 Compare October 27, 2020 20:12
nliber
nliber reviewed Oct 27, 2020
View reviewed changes
Copy link
Contributor

@nliber nliber left a comment •
edited

Choose a reason for hiding this comment

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

The conversion works (assuming the size of the resulting array is large enough, which should be checked), but it is hard to follow because of the non-local nature of the IndexType check.

@dalg24
Copy link
Member Author

dalg24 commented Oct 28, 2020

Retest this please

crtrott
crtrott requested changes Oct 29, 2020
View reviewed changes
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Putting a hold on it: wasn't there some convention regarding death test naming we tried to follow? I thought we had some issues with death tests on various platforms and want an easy way to exclude death tests.

@dalg24
Copy link
Member Author

dalg24 commented Oct 29, 2020

Putting a hold on it: wasn't there some convention regarding death test naming we tried to follow? I thought we had some issues with death tests on various platforms and want an easy way to exclude death tests.

Good catch. Fixed.

@crtrott crtrott merged commit 924a9fb into kokkos:develop Oct 29, 2020
@dalg24 dalg24 deleted the checked_narrowing_conversion branch October 29, 2020 18:39
@ndellingwood ndellingwood mentioned this pull request Nov 2, 2020
Closed
@dalg24 dalg24 mentioned this pull request Nov 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masterleinad masterleinad masterleinad left review comments

@nliber nliber nliber approved these changes

@DavidPoliakoff DavidPoliakoff DavidPoliakoff approved these changes

@crtrott crtrott crtrott 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.

None yet
5 participants
@dalg24 @DavidPoliakoff @nliber @masterleinad @crtrott