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.

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

Validate storage level argument passed to TeamPolicy::set_scratch_size() #3098

Merged
merged 5 commits into from
Jun 18, 2020

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jun 12, 2020

Fix #3097

@dalg24 dalg24 requested a review from dhollman June 12, 2020 19:41
@masterleinad
Copy link
Contributor

Retest this please.

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2020

Codecov Report

Merging #3098 into develop will decrease coverage by 0.0%.
The diff coverage is 66.6%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #3098     +/-   ##
=========================================
- Coverage     85.7%   85.7%   -0.1%     
=========================================
  Files          122     122             
  Lines        10385   10394      +9     
=========================================
+ Hits          8906    8911      +5     
- Misses        1479    1483      +4     
Flag Coverage Δ
#clang 76.2% <ø> (+<0.1%) ⬆️
#gcc 86.4% <66.6%> (-0.1%) ⬇️
Impacted Files Coverage Δ
core/src/Kokkos_ExecPolicy.hpp 91.1% <66.6%> (-1.8%) ⬇️
core/src/impl/Kokkos_HostThreadTeam.cpp 96.0% <0.0%> (-0.8%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f48b709...1d55876. Read the comment docs.

@dalg24
Copy link
Member Author

dalg24 commented Jun 17, 2020

Retest this please

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Apart from the typo that looks good to me.

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Copy link

@dhollman dhollman left a comment

Choose a reason for hiding this comment

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

LGTM, but could be a little better probably.

Comment on lines 594 to 597
std::stringstream ss;
ss << "TeamPolicy::set_scratch_size(/*level*/ " << level
<< ", ...) storage level argument must be 1 or 2 to be valid\n";
Impl::throw_runtime_exception(ss.str());

Choose a reason for hiding this comment

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

Suggestion: move this to a function named throw_invalid_storage_exception(level) or something like that and move it's implementation to Kokkos_ExecPolicy.hpp. Then remove #include <sstream> from this header file (it's just a start, but we have to start somewhere. Throwing invalid scratch size policy exceptions isn't performance-sensitive enough that it needs to be inlined.)

Choose a reason for hiding this comment

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

(IMHO, the if statement itself should stay in a header file, since inlining it doesn't require any additional header files and it's used far more than the body of the if statement)

Copy link
Member Author

Choose a reason for hiding this comment

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

I turned it into a free function and move the implementation to impl/Kokkos_ExecPolicy.cpp.
I decided to keep the if-statement because it did not feel right to separate things after I realized the error message said "must be 1 or 2" instead of "0 or 1"

core/src/Kokkos_ExecPolicy.hpp Outdated Show resolved Hide resolved
@dalg24 dalg24 requested a review from dhollman June 17, 2020 22:14
dalg24 and others added 2 commits June 17, 2020 18:17
…lementation out of the header

Co-Authored-By: D. S. Hollman <dshollm@sandia.gov>
@dalg24 dalg24 merged commit d25d610 into kokkos:develop Jun 18, 2020
@dalg24 dalg24 deleted the scratch_level branch June 18, 2020 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants