-
Notifications
You must be signed in to change notification settings - Fork 40
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
Remove old reduction "tuning" from OpenMP Target variants #484
Remove old reduction "tuning" from OpenMP Target variants #484
Conversation
…rm-ompt-reduction-tunings
I don't know how this code compiled.
RAJA::forall<RAJA::omp_target_parallel_for_exec<threads_per_team>>( | ||
RAJA::RangeSegment(ibegin, iend), | ||
RAJA::expt::Reduce<RAJA::operators::plus>(&tsum), | ||
[=] (Index_type i, Real_type& sum) { |
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.
Real_type& sum
would need to be RAJA::expt::ValOp<Real_type, RAJA::operators::plus> & sum
if we are pulling in the latest changes from RAJA/develop.
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.
We are not on RAJA develop yet. That will come later. One chunk of changes at a time.
m_vmin = RAJA_MIN(m_vmin, static_cast<Int_type>(tvmin)); | ||
m_vmax = RAJA_MAX(m_vmax, static_cast<Int_type>(tvmax)); |
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.
I'm sure there's a good reason, but why are we doing min and max operations outside of the forall here?
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.
Technically, it shouldn't matter when the kernel runs multiple times, but we want to make sure we are not just seeing the result from the last run through the kernel. This change was not made in this PR. It's been like that since the beginning.
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.
Ok, this looks fine because we're using the macros on integer types. If this were inside a lambda, we'd need to use .min()
on the ValOp
type in the near future.
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.
Yup. Those changes will be coming.
Summary