Add bound checks in RangePolicy and MDRangePolicy #6617

ldh4 · 2023-11-22T04:10:26Z

Resolves #6616.

Added a simple check in MDRangePolicy that checks if any of the end index is less than its corresponding start index for all ranks.

core/src/KokkosExp_MDRangePolicy.hpp

masterleinad · 2023-11-25T18:36:49Z

core/src/KokkosExp_MDRangePolicy.hpp

+      if (m_upper[i] < m_lower[i]) {
+        std::stringstream msg;
+        msg << "Kokkos::MDRangePolicy bounds error: The lower bound ("
+            << m_lower[i] << ") is greater than its upper bound (" << m_upper[i]
+            << ") in rank " << i + 1 << ".";
+        Kokkos::abort(msg.str().c_str());
+      }


Did you consider just treating it as an empty range if the upper bound is lower than the lower bound?
Isn't that what we are doing for RangePolicy?

I didn't realize that RangePolicy behaves this way, but the idea of silently changing a policy to be an empty range seems unhelpful for the users, although I admit that given a lower bound that's higher than its upper bound, making it an empty range is most logical.

In case of MDRangePolicy, since the lower bound and upper bound checks will be needed for all ranks, should all ranges become empty ranges upon finding a failed bound check or should only failing ranks be empty ranges?

I personally think we should do something that is the most useful for the users, which IMO is catching the problem as early as possible. Adopting the solution proposed in this PR seems good to me because the problem is caught immediately during the construction of the policy.

FWIW, at first I was also thinking that aborting was the right thing to do; but just emptying the range is also a good solution, afterall when doing for(int i=0; i<-10;i++) this is valid code that may happen when implementating an algorithm where the bound are computed using analytical expressions, and the desired behavior is doing nothing.

"when implementating an algorithm where the bound are computed using analytical expressions, and the desired behavior is doing nothing." what do you mean by:

using analytical expressions,

the desired behavor is doing nothing

I think we need to differentiate what concerns the algirithm vs how the MDRangePolicy works.
I don't think it would be right to tie the implementation/semantics of the MDRangePolicy to something that pertains a certain algorithm.
It should be up to the algorithm to translate its requirements into something that makes sense for MDRangePolicy.
if you algorithm has meaning for a range like [15, -20], then it should be up to you to translate that into an empty MDRangePolicy if you want it do nothing.
IMO, the MDRangePolicy should either accept a valid set of bounds or empty bounds but NOT "flipped" bounds.
Does it make sense what I am trying to convey?

Another option is to state clearly somewhere that the behavior of MDRangePolicy is such that:

empty bounds yield empty md range

valid bounds yield valid md range

invalid bounds (like above) are internally transformed into empty md range

and define what is valid and invalid. But now we get into a rabbit hole, bc for example what if one rank has valid bounds and one rank does not? do you treat the whole thing an empty md range?

I agree with your last option (3 bullets above) and that it should be clearly stated in the documentation.
IMHO, if one rank has upper bound < lower bound then the right behavior, is don't do any iteration at all; afterall this is what happen when you have two nest for loops, and one of them has invalid range.

Somehow, about your 3rd bullet above, when upper bound is < lower bound, it is not really an invalid range, just an empty range, but for a different reason.

The last point is that Range and MDRange policy should behave the same. If you think aborting is best in case of invalid range in MDRange, then Range policy should behave likewise.

Finally, I don't have strong opinion here, just saying that first I was thinking aborting was the right behavior, but finally I can live fine with emptying the range.

Maybe, we just discuss it on Wednesday in the meeting.

core/src/KokkosExp_MDRangePolicy.hpp

…ds are less than its upper bound

core/src/KokkosExp_MDRangePolicy.hpp

dalg24 · 2023-12-01T14:13:10Z

core/unit_test/TestMDRangePolicyConstructors.hpp

@@ -93,6 +93,27 @@ TEST(TEST_CATEGORY_DEATH, policy_bounds_unsafe_narrowing_conversions) {
      },
      "unsafe narrowing conversion");
 }
+
+TEST(TEST_CATEGORY_DEATH, policy_invalid_bounds) {
+  using Policy = Kokkos::MDRangePolicy<TEST_EXECSPACE, Kokkos::Rank<5>>;


Why rank 5 if we can trigger with 2?

No real reason. Changed to 2 ranks and removed the second test case.

dalg24 · 2023-12-01T20:25:35Z

core/unit_test/TestMDRangePolicyConstructors.hpp

+      },
+      "Kokkos::MDRangePolicy bounds error: The lower bound \\(100\\) is "
+      "greater "
+      "than its upper bound \\(90\\) in dimension 1\\.");


I hope the test fails...

(should fail in dimension 0 no?)

The failing dimension depends if you use Iterate::Left or Iterate::Right

So we expect the test as is should not pass on all backends?

Yes, the test should pass for some backends and fail for others

Have we decided anyway that the bounds check should always produce an error?

(should fail in dimension 0 no?)

@dalg24
@Rombur is correct here. ~~It's Iterate::Right by default here. So the check would start from rank-1 dimension first.~~
Looks like I didn't think this all the way through. I'll modify this based on the backend.

@masterleinad No, we haven't decided on that. If we do an abort here for MDRangePolicy, it's inconsistent from how RangePolicy handles this, so I think we definitely should discuss at some point before merging.

I would vote to abort and also change range policy to reflect this for the same reason of what I said in the other comment

Fixed to check against the policy's iteration pattern.

I would vote to abort and also change range policy to reflect this for the same reason of what I said in the other comment

I agree with this approach as well. My only concern is if that will cause many breakages for the users. Although the change is for a corner case, it's a sudden behavioral change in a commonly used policy.

dalg24

Please update the API doc to clearly state that begin[d] <= end[d] for all dimensions is a precondition for the constructors.

core/unit_test/TestMDRangePolicyConstructors.hpp

dalg24 · 2024-01-09T02:43:12Z

If merged, must appear in the changelog under "incpompatibilies"

dalg24 · 2024-01-09T02:54:34Z

Also if we proceed with this PR, we'd probably want to also enforce the precondition for RangePolicy.

dalg24 · 2024-01-09T02:55:32Z

We might need to guard against KOKKOS_ENABLE_DEPRECATED_CODE_4 to be safe.

Formatting. Co-authored-by: Damien L-G <dalg24+github@gmail.com>

ldh4 · 2024-01-09T17:55:53Z

Also if we proceed with this PR, we'd probably want to also enforce the precondition for RangePolicy.

Yes, we still need to have a discussion on that to come to a decision.
For consistency, either RangePolicy will need to have a similar precondition with an abort or MDRangePolicy will need to use a silently emptied range upon violating its precondition.

core/src/KokkosExp_MDRangePolicy.hpp

ldh4 · 2024-01-11T16:50:18Z

Updated RangePolicy to enforce the same precondition.

dalg24 · 2024-01-11T22:19:00Z

core/src/Kokkos_ExecPolicy.hpp

    set_auto_chunk_size();
  }

  /** \brief  Total range */
  inline RangePolicy(const member_type work_begin, const member_type work_end)
      : RangePolicy(typename traits::execution_space(), work_begin, work_end) {
+    check_bounds_validity();


You'd never get here since the construction is deferred to the other RangePolicy constructor that would error out.

I did notice it before, but I placed it in just to have this check logic be paired with each set_auto_chunk_size() call, which makes uses of m_begin and m_end.
If you feel strongly that we don't want this in this constructor (and in the other similar constructor that takes in Args...), I'll remove the checks from the two constructors.

I can live with it. There are things that bother me more in that file :)

ldh4 · 2024-01-16T01:42:06Z

Retest this please.

dalg24 · 2024-01-17T22:20:03Z

CI failures are unrelated

fnrizzi reviewed Nov 22, 2023

View reviewed changes

core/src/KokkosExp_MDRangePolicy.hpp Outdated Show resolved Hide resolved

masterleinad reviewed Nov 25, 2023

View reviewed changes

fnrizzi reviewed Nov 28, 2023

View reviewed changes

core/src/KokkosExp_MDRangePolicy.hpp Outdated Show resolved Hide resolved

fnrizzi reviewed Nov 28, 2023

View reviewed changes

core/src/KokkosExp_MDRangePolicy.hpp Outdated Show resolved Hide resolved

ldh4 added 3 commits November 30, 2023 19:35

Added a bounds check in MDRangePolicy that checks that all lower boun…

8d4a953

…ds are less than its upper bound

Modified the wording on the abort

625c8f1

Converted the error msg from a stringstream to a string

341b464

ldh4 force-pushed the mdrange_add_bounds_check branch from c1e32ce to 341b464 Compare December 1, 2023 02:35

dalg24 reviewed Dec 1, 2023

View reviewed changes

core/src/KokkosExp_MDRangePolicy.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Dec 1, 2023

View reviewed changes

Modified abort msg

a482d4c

dalg24 reviewed Dec 1, 2023

View reviewed changes

Fixed the unit test output based on backend's default iterate direction

307c368

dalg24 approved these changes Jan 9, 2024

View reviewed changes

dalg24 reviewed Jan 9, 2024

View reviewed changes

core/unit_test/TestMDRangePolicyConstructors.hpp Outdated Show resolved Hide resolved

Update core/unit_test/TestMDRangePolicyConstructors.hpp

909c91a

Formatting. Co-authored-by: Damien L-G <dalg24+github@gmail.com>

crtrott reviewed Jan 10, 2024

View reviewed changes

core/src/KokkosExp_MDRangePolicy.hpp Show resolved Hide resolved

Updated RangePolicy to have the same precondition as MDRangePolicy

208bbfb

ldh4 mentioned this pull request Jan 11, 2024

Update RangePolicy and MDRangePolicy on the bounds precondition kokkos/kokkos-core-wiki#485

Merged

dalg24 reviewed Jan 11, 2024

View reviewed changes

ldh4 changed the title ~~Add a bounds check in MDRangePolicy~~ Add bound checks in RangePolicy and MDRangePolicy Jan 16, 2024

tcclevenger approved these changes Jan 17, 2024

View reviewed changes

dalg24 merged commit 179d2e6 into kokkos:develop Jan 17, 2024
30 of 31 checks passed

This was referenced Jan 17, 2024

CHANGELOG: 4.3.0 #6519

Closed

Guard [MD]RangePolicy precondition check for deprecated code 4 #6726

Merged

Improve weak test and make sure OpenACC throws on allocation failures #6729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bound checks in RangePolicy and MDRangePolicy #6617

Add bound checks in RangePolicy and MDRangePolicy #6617

ldh4 commented Nov 22, 2023

masterleinad Nov 25, 2023

ldh4 Nov 28, 2023

fnrizzi Nov 28, 2023

pkestene Nov 28, 2023 •

edited

Loading

fnrizzi Nov 28, 2023

fnrizzi Nov 28, 2023 •

edited

Loading

pkestene Nov 28, 2023

masterleinad Nov 28, 2023

dalg24 Dec 1, 2023

ldh4 Dec 1, 2023

dalg24 Dec 1, 2023

dalg24 Dec 1, 2023

Rombur Dec 1, 2023

dalg24 Dec 1, 2023

Rombur Dec 1, 2023

masterleinad Dec 1, 2023

ldh4 Dec 1, 2023 •

edited

Loading

fnrizzi Dec 1, 2023 •

edited

Loading

ldh4 Dec 1, 2023 •

edited

Loading

dalg24 left a comment

dalg24 commented Jan 9, 2024

dalg24 commented Jan 9, 2024

dalg24 commented Jan 9, 2024

ldh4 commented Jan 9, 2024

ldh4 commented Jan 11, 2024

dalg24 Jan 11, 2024

ldh4 Jan 11, 2024 •

edited

Loading

dalg24 Jan 11, 2024

ldh4 commented Jan 16, 2024

dalg24 commented Jan 17, 2024

Add bound checks in RangePolicy and MDRangePolicy #6617

Add bound checks in RangePolicy and MDRangePolicy #6617

Conversation

ldh4 commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkestene Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnrizzi Nov 28, 2023 • edited Loading

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

Choose a reason for hiding this comment

ldh4 Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

fnrizzi Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

ldh4 Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

dalg24 commented Jan 9, 2024

dalg24 commented Jan 9, 2024

dalg24 commented Jan 9, 2024

ldh4 commented Jan 9, 2024

ldh4 commented Jan 11, 2024

Choose a reason for hiding this comment

ldh4 Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldh4 commented Jan 16, 2024

dalg24 commented Jan 17, 2024

pkestene Nov 28, 2023 •

edited

Loading

fnrizzi Nov 28, 2023 •

edited

Loading

ldh4 Dec 1, 2023 •

edited

Loading

fnrizzi Dec 1, 2023 •

edited

Loading

ldh4 Dec 1, 2023 •

edited

Loading

ldh4 Jan 11, 2024 •

edited

Loading