Skip to content

ROCm Support : Tile_Layout kernel #1201

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

Merged
merged 25 commits into from
Mar 4, 2025
Merged

Conversation

petrex
Copy link
Collaborator

@petrex petrex commented Oct 30, 2024

This pull request introduces support for ROCm (Radeon Open Compute) in addition to CUDA for GPU acceleration. The changes primarily focus on enabling the build and execution of ROCm-specific code paths alongside existing CUDA paths.

In this PR, I use tensor_core_tiled_layout as proof of concept, but it generalizes to other kernels (for example, fp6_llm or sparse_marlin) with minimum effort. Feedback are welcome

co-author : @lcskrishna

Features:

ROCm Support Integration:

  • setup.py: Added detection for ROCm and adjusted the logic for compiling GPU extensions based on the availability of CUDA or ROCm.

Conditional Compilation for ROCm:

These changes ensure that the codebase can compile and run efficiently on both CUDA and ROCm platforms, leveraging the best available GPU acceleration technology.

Usage

With ROCm pytorch nightly docker , simply run PYTORCH_ROCM_ARCH=gfx942 python setup.py install

Next

  • AMD specific unit tests (tensor_core_tiled_layout)
  • workload and platform specific optimization (tensor_core_tiled_layout)

Copy link

pytorch-bot bot commented Oct 30, 2024

🔗 Helpful Links

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

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

❌ 4 New Failures, 1 Pending

As of commit 2272a3f with merge base 9bcd73b (image):

NEW FAILURES - The following jobs have failed:

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 30, 2024
@petrex petrex mentioned this pull request Oct 31, 2024
1 task
@msaroufim msaroufim self-requested a review November 2, 2024 22:50
Copy link
Member

@msaroufim msaroufim left a comment

Choose a reason for hiding this comment

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

Thanks! Left some prelim feedback more of a n00b question but is this the main way you've seen ROCm supported in new codebases? The other approach would be to have some code duplication and avoid a boolean flag like USE_ROCM in the .cu file

setup.py Outdated
@@ -57,11 +59,11 @@ def get_extensions():

if not torch.cuda.is_available():
print("PyTorch GPU support is not available. Skipping compilation of CUDA extensions")
if CUDA_HOME is None and torch.cuda.is_available():
print("CUDA toolkit is not available. Skipping compilation of CUDA extensions")
if CUDA_HOME is None or not IS_ROCM and torch.cuda.is_available():
Copy link
Member

Choose a reason for hiding this comment

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

some of these if conditions might cause our binary builds to break - check CI, since we run binary builds when people modify setup.py

Until you get your first PR merged I'll have to manually approve every CI run so might be easiest if you wanna make some simple doc level PR and I can merge that fast

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for approving the CI flow, we will look into this

@jcaip jcaip mentioned this pull request Nov 11, 2024
3 tasks
@petrex
Copy link
Collaborator Author

petrex commented Nov 13, 2024

@msaroufim can we have the CI flow to test the change? thanks

@petrex petrex changed the title ROCm Support Integration ROCm Support : HIP kernel Generator Nov 13, 2024
@msaroufim
Copy link
Member

I gave you triage access @petrex - what might help is also merging some simple PR since after that you shouldn't need me to trigger CI for you

yanbing-j pushed a commit to yanbing-j/ao that referenced this pull request Dec 9, 2024
@petrex petrex added the topic: new feature Use this tag if this PR adds a new feature label Jan 6, 2025
@petrex
Copy link
Collaborator Author

petrex commented Jan 6, 2025

@msaroufim happy new year! The 2 failing CI check seems irrelevant to this PR? Any tips?

  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +refs/heads/rocm_enablement_staging*:refs/remotes/origin/rocm_enablement_staging* +refs/tags/rocm_enablement_staging*:refs/tags/rocm_enablement_staging*
  Error: The process '/usr/bin/git' failed with exit code 1

@petrex petrex self-assigned this Jan 6, 2025
@petrex petrex requested review from msaroufim and lcskrishna January 8, 2025 15:59
@petrex petrex changed the title ROCm Support : HIP kernel Generator ROCm Support : Tile_Layout Lernel Jan 9, 2025
@jerryzh168
Copy link
Contributor

thanks! this is the dequantize and unpack_dequantize op btw, the packing op and int4mm op lives in pytorch right now: https://github.com/pytorch/pytorch/blob/b1c2c3967adf035356abfcb90d0b39ff7ee63707/aten/src/ATen/native/native_functions.yaml#L4162-L4170

but this can be used to test build for ROCm

@petrex petrex changed the title ROCm Support : Tile_Layout Lernel ROCm Support : Tile_Layout kernel Jan 9, 2025
@petrex petrex requested review from jcaip and removed request for lcskrishna February 27, 2025 20:12
Copy link
Contributor

@jcaip jcaip left a comment

Choose a reason for hiding this comment

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

This looks good, but there are no tests? We should be able to turn on the int4wo tests to run on ROCm to verify this is correct right? Or is there additional work to be done to turn on the test?

Minor variable renaming for clarity in extension directory path resolution
@petrex
Copy link
Collaborator Author

petrex commented Feb 28, 2025

This looks good, but there are no tests? We should be able to turn on the int4wo tests to run on ROCm to verify this is correct right? Or is there additional work to be done to turn on the test?

Thanks , This PR introduced optimized kernel for MI300x. We are working on allocating Mi300x nodes for torchao CI.
Will enable mi300x specific tests in following PRs.

Copy link
Contributor

@jcaip jcaip left a comment

Choose a reason for hiding this comment

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

@petrex OK, that makes sense, approving for now.

We should also run some updated benchmarks now that we have int4+ marlin support. I'll try to find some time next week to do so.

@jcaip
Copy link
Contributor

jcaip commented Feb 28, 2025

cc @petrex I'm assuming you want to merge in this first and then merge #1206 in? In that case I think it should be okay so long as CI passes for 1206 since you already have approval on that PR.

@jcaip jcaip merged commit d4be9e4 into pytorch:main Mar 4, 2025
46 of 52 checks passed
@jcaip jcaip mentioned this pull request Mar 4, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: rocm topic: new feature Use this tag if this PR adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants