Skip to content

[AMD] Turn on fast path for index_put #136136

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
Closed
xw285cornell wants to merge 1 commit into from
Closed

[AMD] Turn on fast path for index_put #136136

xw285cornell wants to merge 1 commit into from

Conversation

@xw285cornell
Copy link
Contributor

@xw285cornell xw285cornell commented Sep 16, 2024

Summary:
This slow path is bad because it has a sync point which makes CPU really slow. I'm not very sure if AMD actually needs this with the newer rocm versino

{F1870213925}

Test Plan: CI

Differential Revision: D62731130

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 16, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/136136

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit c777e69 with merge base 8316f9b (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the release notes: cuda release notes category label Sep 16, 2024
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 16, 2024

This pull request was exported from Phabricator. Differential Revision: D62731130

danzimm
danzimm approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@danzimm danzimm left a comment

Choose a reason for hiding this comment

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

LGTM

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 16, 2024
@jeffdaily jeffdaily added the ciflow/rocm Trigger "default" config CI on ROCm label Sep 16, 2024
@danzimm
Copy link
Contributor

danzimm commented Sep 16, 2024

@jeffdaily does the ciflow/rocm label trigger some additional AMD specific CI?

@jeffdaily
Copy link
Collaborator

jeffdaily commented Sep 16, 2024

@jeffdaily does the ciflow/rocm label trigger some additional AMD specific CI?

We have a limited pool of CI servers. The ciflow/trunk will trigger a fraction of the full CI suite (no inductor, a select set of test modules, etc). OTOH, ciflow/rocm runs much more. We try to limit using ciflow/rocm on PRs because it puts pressure on our CI resources. We have the authority to revert merged PRs if they break ROCm, in which case we add the ciflow/rocm label to the reverted PR to aid the PR author in fixing the issue for ROCm.

@xw285cornell @facebook-github-bot
[AMD] Turn on fast path for index_put (pytorch#136136)
c777e69 
Summary:

This slow path is bad because it has a sync point which makes CPU really slow. I'm not very sure if AMD actually needs this with the newer rocm versino

{F1870213925}

Test Plan: CI

Reviewed By: danzimm

Differential Revision: D62731130
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 15, 2024

This pull request was exported from Phabricator. Differential Revision: D62731130

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 15, 2024

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Oct 15, 2024

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

jackzhxng pushed a commit that referenced this pull request Oct 16, 2024
@xw285cornell @jackzhxng
[AMD] Turn on fast path for index_put (#136136)
fd8b3c5 
Summary:
This slow path is bad because it has a sync point which makes CPU really slow. I'm not very sure if AMD actually needs this with the newer rocm versino

{F1870213925}

Test Plan: CI

Differential Revision: D62731130

Pull Request resolved: #136136
Approved by: https://github.com/danzimm, https://github.com/jeffdaily, https://github.com/eqy
jerrymannil pushed a commit to ROCm/pytorch that referenced this pull request Nov 6, 2024
@xw285cornell @jerrymannil
[AMD] Turn on fast path for index_put (pytorch#136136)
f69efff 
Summary:
This slow path is bad because it has a sync point which makes CPU really slow. I'm not very sure if AMD actually needs this with the newer rocm versino

{F1870213925}

Test Plan: CI

Differential Revision: D62731130

Pull Request resolved: pytorch#136136
Approved by: https://github.com/danzimm, https://github.com/jeffdaily, https://github.com/eqy
jerrymannil pushed a commit to ROCm/pytorch that referenced this pull request Nov 19, 2024
@xw285cornell @jerrymannil
[AMD] Turn on fast path for index_put (pytorch#136136)
25b112a 
Summary:
This slow path is bad because it has a sync point which makes CPU really slow. I'm not very sure if AMD actually needs this with the newer rocm versino

{F1870213925}

Test Plan: CI

Differential Revision: D62731130

Pull Request resolved: pytorch#136136
Approved by: https://github.com/danzimm, https://github.com/jeffdaily, https://github.com/eqy
pruthvistony pushed a commit to ROCm/pytorch that referenced this pull request Mar 4, 2025
@xw285cornell @pruthvistony
[AMD] Turn on fast path for index_put (pytorch#136136)
a02cad7 
Summary:
This slow path is bad because it has a sync point which makes CPU really slow. I'm not very sure if AMD actually needs this with the newer rocm versino

{F1870213925}

Test Plan: CI

Differential Revision: D62731130

Pull Request resolved: pytorch#136136
Approved by: https://github.com/danzimm, https://github.com/jeffdaily, https://github.com/eqy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danzimm danzimm danzimm approved these changes

@jeffdaily jeffdaily jeffdaily approved these changes

@eqy eqy eqy approved these changes

@syed-ahmed syed-ahmed Awaiting requested review from syed-ahmed syed-ahmed is a code owner

Assignees

No one assigned

Labels

ciflow/rocm Trigger "default" config CI on ROCm ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged release notes: cuda release notes category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@xw285cornell @facebook-github-bot @danzimm @jeffdaily @pytorchmergebot @eqy