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

added forall_with_streams and updated BenchmarkForall.cpp #232

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

neelakausik
Copy link
Member

No description provided.

@adayton1
Copy link
Member

Didn't you have a macro in DefaultMacros.h as well?

@neelakausik neelakausik marked this pull request as draft July 19, 2023 23:01
src/care/DefaultMacros.h Outdated Show resolved Hide resolved
forall(gpu_simulation{}, res, fileName, lineNumber, start, end, std::forward<LB>(body));
#elif defined(__CUDACC__)
forall(RAJA::cuda_exec<CARE_CUDA_BLOCK_SIZE, CARE_CUDA_ASYNC>{},
res, RAJA::RangeSegment(start, end), std::forward<LB>(body));
Copy link
Member

Choose a reason for hiding this comment

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

I think you need an overload of forall that takes a resource, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

This is the function you are calling: https://github.com/LLNL/CARE/pull/232/files#diff-1df40e04088de0f82501a0065752487396b8abeb4c3d30780e79119cc63789a7R74

But it does not accept a resource argument. I'm confused at how this is working.

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 thought it was calling RAJA::forall, but will look into it further

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be calling RAJA::forall - there's no overload that takes the fileName and lineNumber.

Copy link
Member

Choose a reason for hiding this comment

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

Also, that's the main reason I dislike "using namespace..." statements - it's too easy to accidentally call the wrong function.

@neelakausik neelakausik marked this pull request as ready for review August 4, 2023 21:19

for (auto _ : state) {
//run num kernels
omp_set_num_threads(16);
Copy link
Member

Choose a reason for hiding this comment

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

This should be N rather than 16.

///
////////////////////////////////////////////////////////////////////////////////
template <typename ExecutionPolicy, typename LB>
void forall(ExecutionPolicy /* policy */, Resource res, const char * fileName, const int lineNumber,
Copy link
Member

Choose a reason for hiding this comment

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

You should add a template parameter as the resource type since we want to be able to accept any resource type.

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.

2 participants