Add support for the new reducer interface to dynamic forall #1759
Conversation
artv3
requested review from
MrBurmark,
adayton1,
rhornung67,
johnbowen42 and
rchen20
Co-authored-by: Robert Chen <[email protected]>
|
@LLNL/raja-core anyone else want to take a look? or are ready to merge? |
|
@artv3 I'd like to look this over before it is merged. |
| if(pol > N-1) { | ||
| RAJA_ABORT_OR_THROW("Policy enum not supported"); | ||
| template<typename POLICY_LIST> | ||
| struct dynamic_helper<0, POLICY_LIST> |
There was a problem hiding this comment.
For a future pass, we should use parameter pack expansion instead of recursive function calls to do the invoke.
| RAJA::expt::dynamic_forall<policy_list>(pol, RAJA::RangeSegment(0, N), [=] RAJA_HOST_DEVICE (int i) { | ||
| RAJA::dynamic_forall<policy_list>(pol, range, | ||
| RAJA::expt::Reduce<RAJA::operators::plus>(&sum), | ||
| RAJA::expt::KernelName("RAJA dynamic forall"), |
There was a problem hiding this comment.
Should we pull off more of the band-aid and move KernelName out of the expt namespace?
There was a problem hiding this comment.
KernelName functions within the new reducer framework, so I think we need to keep it in expt until the new reducers are fully integrated.
There was a problem hiding this comment.
It seems like it should work in kernels without reductions also. Maybe that's a topic for a later discussion.
There was a problem hiding this comment.
I'm using the KernelName with the RAJA caliper integration here: #1773. I would be in favor in getting it out of expt to prompt folks to start using it more confidently.
There was a problem hiding this comment.
It seems like it should work in kernels without reductions also. Maybe that's a topic for a later discussion.
It will, but it requires the same machinery as the new-style reductions. If that machinery, or the interface to it, is experimental then this probably should still be too. Otherwise it's rather innocuous.
There was a problem hiding this comment.
Thats a good point, I agree with @trws. It should stay in expt until we move the new reducer interface out of expt.
|
|
||
| int sum = 0; | ||
| using VAL_INT_SUM = RAJA::expt::ValOp<int, RAJA::operators::plus>; |
There was a problem hiding this comment.
Why do we need this? The way the machinery works on the back, it would be just as easy to hand the lambda a reference to an int. Is this to prevent users from using a different operator on the target or something? Either way shouldn't hold this PR up, but it would probably help with the verbosity of these reductions a significant amount.
There was a problem hiding this comment.
We've recently modified the new-reducer interface to accept only these ValOp objects. This allows the operations within the lambda to be consistent, e.g. valopobj.minloc(array, loc); rather than the macros for loc reductions we were using before. It also allows us to ensure some type and operator safety among the possible reduction objects, e.g. a sum reducer cannot call min(). The ValOp objects take references to the original int or location, so nothing functionally changes for the user outside of the RAJA lambda. Yes, it's more verbose and somewhat ugly, but if nvcc were to allow auto deduction of types in lambda parameters, then we could get rid of some of this verbosity.
There was a problem hiding this comment.
Ok, I see where you're coming from. On the *loc objects, it makes sense to me to have a helper type for those that has an <op>loc or similar on it, but why not allow the regular reference for the single-component types? It's sometimes useful to do different operations in the body than at the actual reduction. Usually that's for things like getting a per-thread value, like "max iterations computed by one thread" or similar, but it does occasionally come up.
To be explicit, I think the ValOp types are a good idea like the strongly-typed indexes, but it seems like something that should be optional rather than required. Also, I would expect to be able to get it by doing RAJA::expt::Reduce<RAJA::operators::plus, int>::arg_type, or decltype(RAJA::expt::Reduce<RAJA::operators::plus>(&sum))::arg_type, or similar so the declaration for the type used to pass in the reducer provides the appropriate ValOp type.
There was a problem hiding this comment.
I think these are good topics, I can capture the comments here as an issue for further discussion. But I think the topic here may be out of the scope for this PR?
|
To double check, Is this ready for merging? It sounds like we are keeping the expt namespace on KernelNaming. |
|
unfortunately the CI is giving me some trouble |
|
@artv3 I merged my SYCL PR after checks passed and merged develop into this PR which restarted CI checks. Hopefully, it will come back green. |
|
This omp target test keeps failing: |
|
@artv3 this is good to merge now. Restarting the OpenMP target test job on lassen usually resolves the issue. I don't know why this test fails intermittently. Failure is more common in CI than running manually and the failure is not predictable. We could consider disabling that kernel for CI. |
Summary
This PR adds support for the new reducer interface to dynamic forall and brings it out of experimental.