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

Add resource argument to PluginStrategy methods #1775

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

johnbowen42
Copy link
Contributor

Summary

  • This PR is a (refactoring, bugfix, feature, something else)
  • It does the following (modify list as needed):
    • Modifies/refactors (class or method) (how?)
    • Fixes (issue number(s))
    • Adds (specific feature) at the request of (project or person)

Design review (for API changes or additions---delete if unneeded)

On (date), we reviewed this PR. We discussed the design ideas:

  1. First idea or goal
  2. Second idea
  3. Third idea

This PR implements 1. and 3. It leaves out 2. for the following reasons

  • (impractical)
  • (too big)
  • (not a good idea anyway)

@johnbowen42 johnbowen42 requested a review from artv3 November 26, 2024 18:14
Comment on lines +271 to +272
// todo(bowen) do we want default resource here?
util::callPreCapturePlugins(context, resource_type::get_default());
Copy link
Member

Choose a reason for hiding this comment

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

Let's think about this

@@ -229,24 +229,24 @@ void launch(LaunchParams const &launch_params, const char *kernel_name, ReducePa
auto&& launch_body = expt::get_lambda(std::forward<ReduceParams>(rest_of_launch_args)...);

//Take the first policy as we assume the second policy is not user defined.
//We rely on the user to pair launch and loop policies correctly.
//We rely on the user to pair launch and loop policies core_protly.
Copy link
Member

Choose a reason for hiding this comment

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

spell correctly?

@@ -14,8 +14,8 @@ class CounterPlugin :
public RAJA::util::PluginStrategy
{
public:
void preCapture(const RAJA::util::PluginContext& p) override {
if (p.platform == RAJA::Platform::host)
void preCapture(const RAJA::util::PluginContext& p, const RAJA::resources::Resource&) override {
Copy link
Member

Choose a reason for hiding this comment

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

Let's double check that we don't need a non-const resource reference.

@artv3
Copy link
Member

artv3 commented Nov 27, 2024

Thanks for working on this, on my side I need a small modification on the PluginContext as its where I am storing the kernel name for the caliper integration. See here: #1773.

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.

3 participants