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

Draft: Valloc Interface for New Reducer #1690

Closed
wants to merge 5 commits into from

Conversation

rchen20
Copy link
Member

@rchen20 rchen20 commented Jul 12, 2024

Summary

  • This PR is a refactoring

  • It does the following:

    • Refactors new reduction interface to have consistent operator semantics
    • Introduces type safety to reduction operators
    • Will require the use of additional types for reduction targets
  • sequential

  • CUDA

  • HIP

  • SYCL

  • OpenMP

using REF_INT_SUM = RAJA::expt::ValOp<int, RAJA::operators::plus>;
using REF_INT_MIN = RAJA::expt::ValOp<int, RAJA::operators::minimum>;
using REF_INT_MAX = RAJA::expt::ValOp<int, RAJA::operators::maximum>;
using REFLOC_INT_MIN = RAJA::expt::ValLocOp<int, RAJA::operators::minimum>;
Copy link
Member

@MrBurmark MrBurmark Jul 12, 2024

Choose a reason for hiding this comment

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

Do we have an option to change the loc type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean you want something like ValLocOp<double, int, minimum>, where int represents the loc type? I don't think we've had that in the past.

Copy link
Member

@MrBurmark MrBurmark Jul 16, 2024

Choose a reason for hiding this comment

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

yep, and we always should have had it.


value_type *target = nullptr;
value_type val = op::identity();
//value_op *vop = nullptr;

#if defined(RAJA_CUDA_ACTIVE) || defined(RAJA_HIP_ACTIVE) || defined(RAJA_SYCL_ACTIVE)
Copy link
Member

Choose a reason for hiding this comment

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

This reminds me that we should specialize SoAPtr on ValLoc, etc.

template<typename EXEC_POL, template <typename, typename, typename> class OP, typename T>
camp::concepts::enable_if< std::is_same< EXEC_POL, RAJA::seq_exec> >
init(Reducer<OP<ValOp<ValLoc<T>,OP>, ValOp<ValLoc<T>,OP>, ValOp<ValLoc<T>,OP>>, ValOp<ValLoc<T>,OP>>& red) {
red.val = OP<ValOp<ValLoc<T>,OP>, ValOp<ValLoc<T>,OP>, ValOp<ValLoc<T>,OP>>::identity();
Copy link
Member

Choose a reason for hiding this comment

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

We should figure out why using Op<ValOp<ValLoc<T>,OP>, ...>::identity() works here but Op<ValOp<T,OP>, ...>::identity() doesn't work below.

@rchen20
Copy link
Member Author

rchen20 commented Aug 6, 2024

After some discussion with @MrBurmark, we've both concluded that the GPU backends for a valloc interface have enough complications that will require a re-design of valloc. The problem lies in the fact that the GPU backend implementation of grid_reduce (e.g. policy/cuda/reduce.hpp) relies on the reduction object and the val-x type (T). In the current valloc design, the val-x type can become a complex type (ValLoc, ValOp, ValOp), which was not the intended use for grid_reduce which primarily should handle basic data types. To remedy this, I will change the valloc interface requirements, and attempt to hide the various val-x types within RAJA, rather than exposing them to the user. Here is an example of the new valloc we have in mind:

Original Valloc Design -

using REF_INT_SUM = RAJA::expt::ValOp<int, RAJA::operators::plus>;
using REFLOC_INT_MIN = RAJA::expt::ValLocOp<int, RAJA::operators::minimum>;
REF_INT_SUM rcuda_sum(0);
REFLOC_INT_MIN rcuda_minloc(std::numeric_limits<int>::max(), -1);
RAJA::forall<EXEC_POL3R>(cuda_res, arange,
  RAJA::expt::Reduce(&rcuda_sum),
  RAJA::expt::Reduce(&rcuda_minloc),
  [=] RAJA_DEVICE (int i, REF_INT_SUM &_rcuda_sum, REFLOC_INT_MIN &_rcuda_minloc){ // lambda body } );

New Valloc Design -

int rcuda_sum(0);
RAJA::expt::ValLoc<int> rcuda_minloc(std::numeric_limits<int>::max(), -1);
RAJA::forall<EXEC_POL3R>(cuda_res, arange,
  RAJA::expt::Reduce<RAJA::operators::plus>(&rcuda_sum),
  RAJA::expt::Reduce<RAJA::operators::minimum>(&rcuda_minloc),
  [=] RAJA_DEVICE (int i, RAJA::expt::reduceref &_rcuda_sum, RAJA::expt::reduceref &_rcuda_minloc){ // lambda body } );

Notes on new design -
It looks like the first new reducer design, except that the lambda data types are references to the underlying data. The lambda data type RAJA::expt::reduceref will internally generate the various val-x types we need, depending on the Reduce policies and concrete data types passed in. reduceref will ensure the type safety and interface consistency that we wanted in the beginning.

@artv3
Copy link
Member

artv3 commented Aug 7, 2024

@rchen20 @MrBurmark Will we update all the other reducers as well?

@rchen20
Copy link
Member Author

rchen20 commented Aug 7, 2024

@rchen20 @MrBurmark Will we update all the other reducers as well?

No, I'm fairly certain this interface change can be contained to the new reductions. Although, later on when we unify the old and new reducers, we can consider how that interface will look, and how we want to implement it. For this implementation, I will probably use some of the principles from @MrBurmark's multi-reducer design.

@artv3
Copy link
Member

artv3 commented Aug 7, 2024

@rchen20 @MrBurmark Will we update all the other reducers as well?

No, I'm fairly certain this interface change can be contained to the new reductions. Although, later on when we unify the old and new reducers, we can consider how that interface will look, and how we want to implement it. For this implementation, I will probably use some of the principles from @MrBurmark's multi-reducer design.

Right that is what I meant, like the SumReducer etc...

@rchen20
Copy link
Member Author

rchen20 commented Sep 6, 2024

Closing in favor of the more updated version here #1732.

@rchen20 rchen20 closed this Sep 6, 2024
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.

4 participants