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

Fp8 hipstream fix #3127

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

acoskunses-AMD
Copy link
Contributor

Pick hip stream where cuda is not defined

Copy link

netlify bot commented Sep 12, 2024

Deploy Preview for pytorch-fbgemm-docs ready!

Name Link
🔨 Latest commit e94d9e9
🔍 Latest deploy log https://app.netlify.com/sites/pytorch-fbgemm-docs/deploys/66ecaf09cfcdb00008409ff6
😎 Deploy Preview https://deploy-preview-3127--pytorch-fbgemm-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@acoskunses-AMD
Copy link
Contributor Author

@jianyuh @jwfromm

@xw285cornell
Copy link
Contributor

We use hipify script for this, is the change here needed?

@jianyuh
Copy link
Member

jianyuh commented Sep 19, 2024

Rebase to resolve conflicts: also as Xiaodong mentioned, internally we don't need this (automatically hipify). Is this needed on OSS workflow? I guess hipify torch is not enabled?

@@ -12,10 +12,13 @@
#include <numeric>

#include <ATen/ATen.h>
#if !defined(USE_ROCM)
Copy link
Contributor

Choose a reason for hiding this comment

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

hipify script should hipify .h right?

Copy link
Contributor

Choose a reason for hiding this comment

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

in any case, I think maybe we can just remove this block just to be clean.

@@ -12,11 +12,14 @@
#include <numeric>

#include <ATen/ATen.h>
#include <c10/hip/HIPStream.h>
#if !defined(USE_ROCM)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this block even needed? If USE_ROCM is not defined, it'll basically be an empty file. So we can just remove this block here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just remove this include and move it down to the USE_ROCM. No need to include cuda header here.

@@ -12,10 +12,13 @@
#include <numeric>

#include <ATen/ATen.h>
#if !defined(USE_ROCM)
Copy link
Contributor

Choose a reason for hiding this comment

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

in any case, I think maybe we can just remove this block just to be clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants