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

Dual View Tool Events #3326

Merged
merged 12 commits into from
Sep 11, 2020
Merged

Conversation

DavidPoliakoff
Copy link
Contributor

This PR adds Dual View events to the callback interface, and implements them. Current signature (up for debate) are events on sync and modify that pass

  1. The name of the view (const char*)
  2. The beginning of the allocation (const void* const)
  3. Whether the sync/modify was on the device side (bool: true means on device)

Questions I have for reviewers (other than "should this go in")

  1. Right now we pass the host View name if the sync/modify is on the host, device view name otherwise. I could see it being smart to pass the same name to both events, though I decided not to. Game to hear other opinions though
  2. Are these the only events that matter (pretty sure yes but I don't know Dual Views)?

@stanmoore1
Copy link
Contributor

Yes!!! I've wanted this for forever, see kokkos/kokkos-tools#11.

@DavidPoliakoff
Copy link
Contributor Author

@stanmoore1 , Stan, we're exactly on the same page, I'm just about two years slower. I'm going back through the Kokkos Tools backlog, but we should also have a conversation about anything you want done in the Tools area. The roadmap, fyi:

  1. This
  2. "FakeCuda" memory space (actually host memory, but differentiable by tools as a different MemorySpace)
  3. Memory sampling plus profiling

"If I see an access on the Host copy after a change on the device copy without a sync in the middle" kinds of rules start to become possible. Now I know you've been there for years, but we're catching up!

containers/src/Kokkos_DualView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_DualView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_DualView.hpp Outdated Show resolved Hide resolved
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Please document first what this does. The bool state isn't clear to me. Like why is "onDevice" false when it needed to sync to the host? And why is it true when syncing to the device? I mean what if it is in a valid state either way and so need_sync would be false for both host and device? Is the bool just to mean that foo in sync_foo or modify_foo was device if it is true?

@DavidPoliakoff
Copy link
Contributor Author

@crtrott

Is the bool just to mean that foo in sync_foo or modify_foo was device if it is true?

Yes. Do we want a different name, do we want an enum, or what?

@crtrott
Copy link
Member

crtrott commented Sep 9, 2020

If we just get a comment on the functions what the arguments mean and maybe rename in the sync version the on_device to to_device I am fine.

@DavidPoliakoff
Copy link
Contributor Author

@crtrott, handled via 87c5b2d , if this passes CI

@crtrott crtrott merged commit 7d32f6e into kokkos:develop Sep 11, 2020
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.

None yet

5 participants