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.

Sign up for GitHub

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

Jump to bottom

[XLA:GPU] Add back the profiling information #58500

Merged
merged 2 commits into from Nov 11, 2022
Merged

[XLA:GPU] Add back the profiling information #58500

merged 2 commits into from Nov 11, 2022

Conversation

nouiz
Copy link
Contributor

@nouiz nouiz commented Nov 9, 2022 •
edited

This commit removed the profiling information for some thunks including fused thunks with reduction:

commit 9ac28f43e180e0db65bf7872fcd828680e3d2e92
Author: George Karpenkov <cheshire@google.com>
Date:   Wed Oct 26 06:05:31 2022 -0700

Instead of converting many call of ThunkInfo(op) to GetThunkInfo(op), I changed the API to be simpler. So less chance of breaking this in the future.

This has the side effect that initializer now have profiling information. I consider this a good thing.

@cheshire

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Nov 9, 2022
@google-ml-butler google-ml-butler bot requested a review from r4nt November 9, 2022 19:53
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Nov 9, 2022
@nouiz nouiz changed the title Add back the profiling information [XLA:GPU] Add back the profiling information Nov 9, 2022
@cheshire
Copy link
Member

cheshire commented Nov 9, 2022

Thanks a lot ! Looks like I broke it during refactoring.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 9, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 9, 2022
@nouiz
Make sure that GetThunkInfo is always called. This has a side effect …
1f1b6d7 
…of also calling it for initializer thunk. I think this should be good. We need to know who triggered those kernels.
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Nov 9, 2022
@nouiz
Copy link
Contributor Author

nouiz commented Nov 9, 2022

I updated the PR to something more complete. I didn't fixed all the issues. Now I think I did it.
I updated the description with the new fix.

cheshire
cheshire approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@cheshire cheshire left a comment

Choose a reason for hiding this comment

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

Thanks ! We should really add tests for this in the future, but I'm not really sure how (E.g. somehow using cupti?)

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 10, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 10, 2022
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Nov 10, 2022
@gbaned gbaned added the comp:xla XLA label Nov 10, 2022
@nouiz
Copy link
Contributor Author

nouiz commented Nov 10, 2022

I quick grep found some cupti related tests. So it should be possible to add some tests.

@copybara-service copybara-service bot merged commit 3e3f1ba into tensorflow:master Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheshire cheshire cheshire approved these changes

@r4nt r4nt Awaiting requested review from r4nt

Assignees

@gbaned gbaned

Labels
awaiting review Pull request awaiting review comp:xla XLA ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Assigned Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nouiz @cheshire @kokoro-team @gbaned