Skip to content

Conversation

@fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Oct 24, 2021

Move the ContextState class and the ScopedContext class hierarchy outside of the ALPAKA_ACCELERATOR_NAMESPACE namespace, and template them on the Queue type used by the accelerator.

Move the ProductBase class and Product<T> class template outside of the ALPAKA_ACCELERATOR_NAMESPACE namespace, and template them on the Queue type used by the accelerator.

@fwyzard fwyzard marked this pull request as draft October 24, 2021 14:39
@fwyzard fwyzard added the alpaka label Oct 24, 2021
@fwyzard fwyzard changed the title [alpaka] Various developments to each testing [alpaka] Various developments to ease testing Oct 24, 2021
@fwyzard fwyzard force-pushed the alpaka_developments branch from 366617e to 819deec Compare October 24, 2021 16:07
Copy link
Collaborator

@makortel makortel left a comment

Choose a reason for hiding this comment

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

The "Simplify ScopedContextGetterBase::synchronizeStreams" commit looks good.

The "Template ScopedContext on the Queue type" commit looks generally good too, I left some comments that caught my eye that are related to the Product<T> and possibly were not caught by the compiler because that functionality is not used yet.

auto getEvent(T_Acc acc) {
return getEventCache().get(acc);
}
auto getEvent() { return getEventCache<alpaka::Event<Queue>>().get(device()); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not about this PR, but I thought note since it caught my eye. I don't really understand what this function (in conjunction with the commented destructor body) should do. The ScopedContextProduce should store the std::shared_ptr<Event> (e.g. in its constructor), pass it to all Product<T>'s, and record the event ("enqueue to queue"?) in its destructor. Ok, doesn't really matter in practice at this stage where the Product<T> wrapper is not used yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea... so far I've concentrated on cleaning things up, without breaking anything.
Understanding what is missing will come next...

std::unique_ptr<Product<T>> wrap(T data) {
// make_unique doesn't work because of private constructor
return std::unique_ptr<Product<T>>(new Product<T>(device(), streamPtr(), getEvent(acc), std::move(data)));
return std::unique_ptr<Product<T>>(new Product<T>(streamPtr(), std::move(data)));
Copy link
Collaborator

@makortel makortel Oct 26, 2021

Choose a reason for hiding this comment

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

This should also pass the shared_ptr<Event>, but see my other comment #255 (comment).

@fwyzard fwyzard force-pushed the alpaka_developments branch from 9b16ff0 to a6c56aa Compare October 27, 2021 09:32
@fwyzard fwyzard changed the title [alpaka] Various developments to ease testing [alpaka] Template ContextState and ScopedContext on the Queue type Oct 27, 2021
@fwyzard fwyzard marked this pull request as ready for review October 27, 2021 09:35
Move the ContextState class and the ScopedContext class hierarchy outside of the ALPAKA_ACCELERATOR_NAMESPACE
namespace, and template them on the Queue type used by the accelerator.
@fwyzard fwyzard force-pushed the alpaka_developments branch from a6c56aa to fdc589a Compare October 27, 2021 09:50
@fwyzard fwyzard changed the title [alpaka] Template ContextState and ScopedContext on the Queue type [alpaka] Template varius alpakatools classes on the Queue type Oct 27, 2021
@fwyzard fwyzard force-pushed the alpaka_developments branch from 0f4ca09 to df7c52a Compare October 27, 2021 17:38
@makortel
Copy link
Collaborator

About the "Fix the use of multiple devices in ESProduct" commit, the current model in alpaka (as well as in kokkos) for EventSetup products is actually closer to #257 than that of cuda. I.e. the transfers are issued in the ESProducer, and the EventSetup products just hold the data. Me feeling is that in order to support multiple devices in alpaka, it would be simpler to continue in that direction. The API can (and probably should at this point) be simpler than in #257 since only 3 ESProducers are affected here. The main question to me is whether to make the current ES products explicitly to support multiple devices, or repurpose the ESProduct<T> wrapper for that (in the spirit of #257). Certainly something for a subsequent PR though.

@fwyzard
Copy link
Contributor Author

fwyzard commented Oct 28, 2021

Actually, multiple devices are still not supported, because all device memory operations operate on the first device.
Will fix and update this PR...

Move the ProductBase class and Product<T> class template outside of the ALPAKA_ACCELERATOR_NAMESPACE
namespace, and template them on the Queue type used by the accelerator.
@fwyzard fwyzard force-pushed the alpaka_developments branch from df7c52a to 992a608 Compare October 28, 2021 08:51
@fwyzard fwyzard merged commit d26e4c3 into cms-patatrack:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants