Add checks for unsafe implicit conversions in RangePolicy

[ldh4]

Resolves #6709

Added checks in RangePolicy to detect non value-preserving implicit conversions between input bounds and the policy's IndexType.

Cases considered:

• signed arg type to unsigned index types
• unsigned arg type to signed index types
• Narrowing conversions

[crtrott]

I believe the logic isn't quite right for what it should check. I would suggest we just copy the stuff from mdspan (and backs-porting):

https://github.com/llvm/llvm-project/blob/0e8eb445db0cc2552d9d077b527a43c779785cb9/libcxx/include/__mdspan/extents.h#L228-L252

Also the function for the representability check should only be called in debug mode maybe?

[dalg24]

DongHun and I met earlier today. I shared an alternative implementation https://godbolt.org/z/WEKM444n4 and we rediscovered that MDRange had

kokkos/core/src/KokkosExp_MDRangePolicy.hpp

Lines 74 to 87 in 4c94f08

	// Checked narrowing conversion that calls abort if the cast changes the value
	template <class To, class From>
	constexpr To checked_narrow_cast(From arg) {
	constexpr const bool is_different_signedness =
	(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 &&
	is_less_than_value_initialized_variable(arg) !=
	is_less_than_value_initialized_variable(ret))) {
	Kokkos::abort("unsafe narrowing conversion");
	}
	return ret;
	}

We also discussed that C++20 has std::in_range that does pretty much what we want.

We decided to focus on making sure that we got the tests right and agreed we can refactor as needed later.

[ldh4]

I believe the logic isn't quite right for what it should check. I would suggest we just copy the stuff from mdspan (and backs-porting):
https://github.com/llvm/llvm-project/blob/0e8eb445db0cc2552d9d077b527a43c779785cb9/libcxx/include/__mdspan/extents.h#L228-L252

I think there may be some disparities between what the mdspan extent is requiring and what is currently accepted for indicies in RangePolicy.

For example of this case:

Kokkos::RangePolicy<Kokkos::IndexType<int>> p(-3, -1);

(in godbolt)
Although not a very intuitive example, currently it's valid to pass in negative values as bounds in RangePolicy if IndexType is set as a signed integral type. Also the kokkos documentation on RangePolicy doesn't seem to be requiring IndexType to be a positive value. (IndexType defaults to int64_t)

But looking at the comment in the mdspan extent's representability check, the precondition is that an extent must be a positive integer:

// Function to check whether a value is representable as another type
// value must be a positive integer otherwise returns false

So, there is an upfront check to disallow any negative extents, and because of this requirement, the rest of representability check would allow signed to unsigned conversion (which would be an unsafe implicit conversion for RangePolicy described in #6709 that this PR aims to catch).

I think we would need to establish that RangePolicy should not be taking in negative values for bounds in order to use that mdspan extent check.

[dalg24]

DongHun is correct. std::mdspan says

representable as a nonnegative value of type index_type

and we allow negative values in RangePolicy

[dalg24]

Sometimes it’s best to dismiss the reviewer’s suggestion:)

…

On Thu, Feb 1, 2024 at 7:07 PM Dong Hun Lee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/Kokkos_ExecPolicy.hpp <#6754 (comment)>: > : m_space(work_space), m_begin(work_begin), m_end(work_end), m_granularity(0), m_granularity_mask(0) { +#ifdef KOKKOS_ENABLE_DEBUG Removed this guard and in the unit test. Also the function for the representability check should only be called in debug mode maybe? It was added in attempt to address this feedback. — Reply to this email directly, view it on GitHub <#6754 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACIQEW2J6K7NWNWO54ZTIDYRQU3BAVCNFSM6AAAAABCNAFQS6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJXHE3DSOBQGM> . You are receiving this because you commented.Message ID: ***@***.***>

[masterleinad]

Whether or not we disable deprecation warnings shouldn't affect the interface exposed. Whether we warn or error out is determined in the implementation of check_conversion_safety anyway and there the guards make sense.

Suggested change

	#if !defined(KOKKOS_ENABLE_DEPRECATED_CODE_4) || \
	defined(KOKKOS_ENABLE_DEPRECATION_WARNINGS)
	#ifndef KOKKOS_ENABLE_DEPRECATED_CODE_4

What happens if is_convertible is false anyway? Wouldn't that already result in a compilation error? In that case, we can just enable the new interface unconditionally anyway, right?

edit: I meant always using the new interface instead of the old one.

[dalg24]

Whether or not we disable deprecation warnings shouldn't affect the interface exposed.

I agree with this statement in general. But the change you suggest would inhibit diagnosing narrowing conversions. (I am not saying we cannot come up with something else, just that the suggestion does not work.)

[masterleinad]

Updated my comment.

[ldh4]

Whether we warn or error out is determined in the implementation of check_conversion_safety anyway and there the guards make sense.
What happens if is_convertible is false anyway? Wouldn't that already result in a compilation error? In that case, we can just enable the new interface unconditionally anyway, right?

Yes, since the old interface simply accepted arguments that are implicitly convertible to the member_type, semantically it should be no different from enforcing the explicit is_convertible check to the interface. I placed in those ifdef guards as a form of compatibility safety and mainly to follow our regular process for changing user facing interface.
But I am for using the new interface right away and leave the ifdef gurads only in check_conversion_safety as well.

[ldh4]

Whether or not we disable deprecation warnings shouldn't affect the interface exposed.

I agree with this statement in general. But the change you suggest would inhibit diagnosing narrowing conversions. (I am not saying we cannot come up with something else, just that the suggestion does not work.)

@dalg24, any opinion on replacing the old interface with the new interface right away and leave the two ifdef guards only in check_conversion_saftey?

[dalg24]

@dalg24, any opinion on replacing the old interface with the new interface right away and leave the two ifdef guards only in check_conversion_saftey?

OK with it.

[ldh4]

Switched to use the new interfaces and removed ifdef guards. 3065552.

[dalg24]

Ignoring failure of one of the HIP build (machine issue) and

5: [ RUN      ] cuda.workgraph_fib
5: /var/jenkins/workspace/Kokkos_PR-6754/core/unit_test/TestWorkGraph.hpp:127: Failure
5: Expected equality of these values:
5:   h_values(0)
5:     Which is: 5168
5:   full_fibonacci(m_input)
5:     Which is: 6765
5: [  FAILED  ] cuda.workgraph_fib (1416 ms)