Fix adjacent difference #6922

yasahi-hpc · 2024-04-06T14:27:38Z

Resolves #6890

This PR aims at adding assertion in adjacent_difference to disallow the overlapping of source and destination.
expect_no_overlap is newly added which is enabled in debug mode only. In the debug mode, it checks

// KOKKOS_EXPECTS(first < s_first && s_last < last); // previous implementation
KOKKOS_EXPECTS(first > s_last1 || last < s_first1); // current implementation

It disallows the case 0 but still allows cases 1 and 2.

Case 1 can be disabled if decltype(begin(const_a)) is constructible from decltype(begin(a)).
In order to suppress Case 2, O (n) of comparisons may be needed.

// Case 0 No longer allowed
Kokkos::View<double*> a("A",N0);
auto res1 = KE::adjacent_difference("label", exespace(), a, a, args...);

// Case 1 still allowed
Kokkos::View<const double*> const_a(a);
auto res2 = KE::adjacent_difference("label", exespace(), const_a, a, args...);

// Case 2 still allowed (element b (1, 1) overlaps)
Kokkos::View<double**> b("B",N0, N1);
auto sub_b0 = Kokkos::subview(b, 0, Kokkos::pair<int, int>(1, 5) );
auto sub_b1 = Kokkos::subview(b, Kokkos::pair<int, int>(0, 4), 1 );
auto res3 = KE::adjacent_difference("label", exespace(), sub_b0, sub_b1, args...);

jbigot · 2024-04-06T16:15:58Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

+                                              IteratorType2 s_last) {
+  if constexpr (std::is_constructible_v<IteratorType2, IteratorType1>) {
+    IteratorType1 s_first1(s_first), s_last1(s_last);
+    KOKKOS_EXPECTS(first < s_first1 && s_last1 < last);


don't you want to check instead:

( first > s_last1 ) || ( last < s_first1 )

?

That is better. I fixed accordingly

masterleinad · 2024-04-08T12:22:43Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

+                                              IteratorType2 s_last) {
+  if constexpr (std::is_constructible_v<IteratorType2, IteratorType1>) {
+    IteratorType1 s_first1(s_first), s_last1(s_last);
+    KOKKOS_EXPECTS(first > s_last1 || last < s_last1);


Suggested change

KOKKOS_EXPECTS(first > s_last1 || last < s_last1);

KOKKOS_EXPECTS(first > s_last1 || last < s_first1);

Are these checks correct for strided Views?

If the iterators are convertible to each other, they should refer to the same stride, no?

I was imagining a scenario where you could have iterators of the same type with the same stride but different starting pointers so that they don't overlap. For example, the location in memory would be alternating between the first iterator and the second iterator.

True... How do incomparable iterators compare in C++?

I think you can check for all Kokkos::view::iterator in case the referenced view is the same, even with non unit stride. This would case the most common case, wouldn't it?

We need to add a method to Kokkos::iterator to compare internal views?

Yes, Kokkos::Experimental::Impl::RandomAccessIterator. Note that you don't have direct access to its stride but you can get it from

auto next_iterator = iterator; auto stride = &*(++next_iterator) - &*iterator;

(assuming the View access operator returns a reference). Otherwise, you would need to add a get_view member function to RandomAccessIterator to obtain the stride directly.

That is nice. So the check would be something like this? (assuming the View access operator returns a reference)

auto next_first = first; auto stride = &*(++next_first) - &*first; if constexpr (std::is_constructible_v<IteratorType2, IteratorType1>) { IteratorType1 s_first1(s_first), s_last1(s_last); auto first_diff = &*first - &*s_first1; bool is_no_overlap = first_diff % stride; KOKKOS_EXPECTS(first > s_last1 || last < s_first1 || is_no_overlap); }

this does heavily rely on assumptions about the internal working of view iterators (for example the fact that you can not iterate on a 2D view like with a std::ranges::cartesian_product_view) if this was ever to be extended to support it, the hypothesis here would start failing. I feel that making this check function friend of the iterator class would at least make this dependency explicit.

masterleinad · 2024-04-08T12:22:55Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

@@ -215,6 +215,28 @@ KOKKOS_INLINE_FUNCTION void expect_valid_range(IteratorType first,
  (void)last;
 }

+//
+// Check if iterators are overlapped


Suggested change

// Check if iterators are overlapped

// Check if iterators are overlapping

masterleinad · 2024-04-08T12:23:33Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

+    KOKKOS_EXPECTS(first > s_last1 || last < s_last1);
+  } else if constexpr (std::is_constructible_v<IteratorType1, IteratorType2>) {
+    IteratorType2 first2(first), last2(last);
+    KOKKOS_EXPECTS(first2 > s_last || last2 < s_last);


Suggested change

KOKKOS_EXPECTS(first2 > s_last || last2 < s_last);

KOKKOS_EXPECTS(first2 > s_last || last2 < s_first);

masterleinad · 2024-04-08T12:24:39Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

+  // avoid compiler complaining when KOKKOS_EXPECTS is no-op
+  (void)first;
+  (void)last;
+  (void)s_first;
+  (void)s_last;


Suggested change

// avoid compiler complaining when KOKKOS_EXPECTS is no-op

(void)first;

(void)last;

(void)s_first;

(void)s_last;

masterleinad · 2024-04-08T12:25:10Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

+KOKKOS_INLINE_FUNCTION void expect_no_overlap(IteratorType1 first,
+                                              IteratorType1 last,
+                                              IteratorType2 s_first,
+                                              IteratorType2 s_last) {


Suggested change

KOKKOS_INLINE_FUNCTION void expect_no_overlap(IteratorType1 first,

IteratorType1 last,

IteratorType2 s_first,

IteratorType2 s_last) {

KOKKOS_INLINE_FUNCTION void expect_no_overlap([[maybe_unused]] IteratorType1 first,

[[maybe_unused]] IteratorType1 last,

[[maybe_unused]] IteratorType2 s_first,

[[maybe_unused]] IteratorType2 s_last) {

yasahi-hpc · 2024-04-08T13:23:27Z

Hi, @masterleinad and @jbigot

Thank you for your comments. I made a fix

crtrott · 2024-04-08T17:00:42Z

Not sure what the cases are, but I guess the thing this would now "wrongly" complain about is two layout stride views with offset 1 between them and stride 2? Do we care?

crtrott · 2024-04-08T17:01:14Z

otherwise looks good.

yasahi-hpc · 2024-04-08T17:52:07Z

That is true. We can create some valid (non-overlapping) cases with layout stride views, which do not satisfy this check. If there is a way to check that in a reasonable cost (say O (1)), it is certainly preferable to add that test. Maybe we can do some integer problem?

yasahi-hpc · 2024-04-08T18:21:51Z

So, it now suppresses "maybe overlapping" cases for safety.

masterleinad · 2024-04-08T18:59:08Z

That is true. We can create some valid (non-overlapping) cases with layout stride views, which do not satisfy this check. If there is a way to check that in a reasonable cost (say O (1)), it is certainly preferable to add that test. Maybe we can do some integer problem?

See #6922 (comment). I would just test specific types.

masterleinad · 2024-05-16T12:58:25Z

You'll have to rebase on top of develop.

…ination

yasahi-hpc · 2024-05-16T13:38:59Z

@masterleinad Sorry about that. I thought I did that, but something went wrong.

algorithms/src/std_algorithms/impl/Kokkos_AdjacentDifference.hpp

masterleinad · 2024-05-16T14:29:01Z

algorithms/src/std_algorithms/impl/Kokkos_AdjacentDifference.hpp

@@ -82,9 +82,17 @@ OutputIteratorType adjacent_difference_exespace_impl(
    return first_dest;
  }

-  // run
+  // ranges shall not overlap


Suggested change

// ranges shall not overlap

// check for overlapping iterators

masterleinad · 2024-05-16T14:49:06Z

algorithms/src/std_algorithms/impl/Kokkos_AdjacentDifference.hpp

+  ::Kokkos::parallel_for(
+      "Check", RangePolicy<ExecutionSpace>(ex, 0, 1), KOKKOS_LAMBDA(int) {
+        Impl::expect_no_overlap(first_from, last_from, first_dest, last_dest);
+      });


Suggested change

::Kokkos::parallel_for(

"Check", RangePolicy<ExecutionSpace>(ex, 0, 1), KOKKOS_LAMBDA(int) {

Impl::expect_no_overlap(first_from, last_from, first_dest, last_dest);

});

#ifdef KOKKOS_ENABLE_DEBUG

::Kokkos::parallel_for(

"Check", RangePolicy<ExecutionSpace>(ex, 0, 1), KOKKOS_LAMBDA(int) {

Impl::expect_no_overlap(first_from, last_from, first_dest, last_dest);

});

#endif

so we don't have to always pay for the launch overhead for another kernel.

That is true. I also add this guard to the team level, since this check performs some computations

masterleinad · 2024-05-16T14:50:07Z

algorithms/src/std_algorithms/impl/Kokkos_Constraints.hpp

+    if constexpr (std::is_constructible_v<IteratorType2, IteratorType1>) {
+      IteratorType2 first2(first), last2(last);
+      IteratorType2 next_first2           = first2;
+      ptrdiff_t stride                    = &*(++next_first2) - &*first2;


Instead, we could avoid executing this in the correct space if we allow to retrieve the View from the iterator.

This reverts commit 7d4833a.

yasahi-hpc marked this pull request as draft April 6, 2024 14:27

yasahi-hpc changed the base branch from master to develop April 6, 2024 14:29

jbigot reviewed Apr 6, 2024

View reviewed changes

masterleinad reviewed Apr 8, 2024

View reviewed changes

Yuuichi Asahi added 18 commits May 16, 2024 15:07

Add assertion in adjacent_difference with overlapping source and dest…

1ab2111

…ination

Disallow overlap not identical inputs

c49c314

check equality of first and last in advance

4e141d4

Test overlap only if iterators are convertible

4d4b215

add else if for the opposite case

31f107f

fix check for overlapping iterators

1a57bd5

avoid no-op in no-overlap check

88883e1

fix check conditions for overlapping iterators

622adef

Formatting expect_no_overlap

e048376

fix check conditions for overlapping iterators

72ab9b1

fix conflicts

a6884e2

fix conflicts

48a87d1

fix conflicts

fbfe0d4

fix conflicts

6bfec12

avoid no-op in no-overlap check

7522cea

Add maybe unused

fa41a65

Check must be made on exec space

491f668

remove unused lines

a3accb1

yasahi-hpc force-pushed the fix-adjacent_difference branch from 2db8971 to a3accb1 Compare May 16, 2024 13:36

masterleinad reviewed May 16, 2024

View reviewed changes

Yuuichi Asahi added 7 commits May 16, 2024 19:01

Enable checks for debug mode only

951efcb

suppress unnecessary iterator conversions

ad97fe8

improve comments for checks

3929be7

fix iterator overlapping check

207dec1

fix iterator overlapping check

e7dd490

fix iterator overlapping check

7d4833a

Revert "fix iterator overlapping check"

ed95ec7

This reverts commit 7d4833a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix adjacent difference #6922

Fix adjacent difference #6922

yasahi-hpc commented Apr 6, 2024 •

edited

jbigot Apr 6, 2024

yasahi-hpc Apr 8, 2024

masterleinad Apr 8, 2024

masterleinad Apr 8, 2024

jbigot Apr 8, 2024

masterleinad Apr 8, 2024

jbigot Apr 8, 2024

jbigot Apr 9, 2024

yasahi-hpc Apr 9, 2024

masterleinad Apr 9, 2024

yasahi-hpc Apr 9, 2024 •

edited

jbigot Apr 10, 2024

masterleinad Apr 8, 2024

masterleinad Apr 8, 2024

masterleinad Apr 8, 2024

masterleinad Apr 8, 2024

yasahi-hpc commented Apr 8, 2024

crtrott commented Apr 8, 2024

crtrott commented Apr 8, 2024

yasahi-hpc commented Apr 8, 2024

yasahi-hpc commented Apr 8, 2024

masterleinad commented Apr 8, 2024

masterleinad commented May 16, 2024

yasahi-hpc commented May 16, 2024

masterleinad May 16, 2024

masterleinad May 16, 2024

yasahi-hpc May 16, 2024

masterleinad May 16, 2024

	KOKKOS_EXPECTS(first > s_last1 \|\| last < s_last1);
	KOKKOS_EXPECTS(first > s_last1 \|\| last < s_first1);

	// Check if iterators are overlapped
	// Check if iterators are overlapping

	KOKKOS_EXPECTS(first2 > s_last \|\| last2 < s_last);
	KOKKOS_EXPECTS(first2 > s_last \|\| last2 < s_first);

	// ranges shall not overlap
	// check for overlapping iterators

Fix adjacent difference #6922

Are you sure you want to change the base?

Fix adjacent difference #6922

Conversation

yasahi-hpc commented Apr 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc commented Apr 8, 2024

crtrott commented Apr 8, 2024

crtrott commented Apr 8, 2024

yasahi-hpc commented Apr 8, 2024

yasahi-hpc commented Apr 8, 2024

masterleinad commented Apr 8, 2024

masterleinad commented May 16, 2024

yasahi-hpc commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasahi-hpc commented Apr 6, 2024 •

edited

yasahi-hpc Apr 9, 2024 •

edited