-
Notifications
You must be signed in to change notification settings - Fork 407
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
MDRange Tile Size Tuning #3688
MDRange Tile Size Tuning #3688
Conversation
Blocked on my ability to understand this error: https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/Kokkos/detail/Kokkos/3941/pipeline |
Looks like you are trying to invoke a rank 3 thing with only two indicies somewhere? |
How so? The MDRange being pointed out is 2d, no? |
@masterleinad : https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/Kokkos/detail/Kokkos/4043/pipeline/43 the error persist after merging latest |
Hmmm... I guess you don't have a configuration at hand that would reproduce the error? |
I can build one. Is it about seeing the skipped contexts? |
No, the changes here are small enough that it should not be too hard to figure out what exactly is breaking it by selectively disabling changes. |
Also, since I made it, here's a build with
|
Hmmm.. now
|
I can fix it, I probably forgot an overload |
Yup, I forgot to change another form of bounds calculator to match the new interface, fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments. Otherwise, the changes look reasonable to me.
@masterleinad : done, thanks, Worked on my build, will see how CI treats it |
You need to fix indentation. |
This apparently needs another round of fixes to adjust for #3626 |
Sorry 😕 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please post a use example in the discussion
@dalg24, what do you mean by
? Any MDRangePolicy kernel without a specified tile size will exercise this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have something like:
#if CXXSTD >= 17
# define KOKKOS_IF_CONSTEXPR if constexpr
#else
# define KOKKOS_IF_CONSTEXPR if
#endif
?
Because a few things like: if(should_tune(policy))
appear to be compile-time constants and in that case, we should use if constexpr
when available
Please cleanup history and I will merge |
Hey @jrmadsen , those actually won't be compile_time, but it's a good thought if we can. "should_tune" just checks that a given instance of a policy should be tuned, which depends on runtime parameters (in the TeamPolicy case, was either parameter declared "AUTO". In the MDRange case, was the tile size unspecified?) @dalg24, will do, thanks |
065fead
to
a8ace51
Compare
@dalg24 cool, passed CI, this is ready to merge in my book |
Implements #3156
If a user doesn't specify a tile size, this PR will add tuning hooks to enable that tuning.
Calling out a few oddities
init
, which I missed.