Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ROCm changes #1102
ROCm changes #1102
Changes from 45 commits
59267a9
a223936
4610075
a506c52
f596bde
0cfb792
f13af44
25e5b71
dcbe19f
fda048e
00abba1
cf307b6
2d66ea8
958679b
e642a48
d0d294a
146f2df
eb0cf36
0c86f2b
6e7f13e
edd3306
9a45f4a
309a3a1
358eaf5
3a915a8
40928ba
69abf78
5c0096e
bfac874
1cf7e84
5b33287
2c514c5
0d5a012
ae14a47
1718605
bc902a3
0d95948
9a5a33b
6490dbc
77627ae
18b48e9
99a70e1
fed56ff
4b39a70
785afb8
bbd0ad1
9db83d8
eabd0a8
2038008
0202078
9cf8856
b885322
0e3dfdb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is AVX512 related to CUDA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AMD CPU has no support for avx512, and the compiler will complain if adding the compilation flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, HIP is for AMD GPUs so it is irrelevant. For example, I believe we can use AMD GPUs with an Intel Skylake CPU, which has AVX512.
At least, it is not related to CUDA. Can I ask you to fix properly (e.g., detecting the availability of AVX-512 instructions)?
(@jianyuh Please teach me how FBGEMM should handle AVX-512.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good point. For current FBGEMM CPU build, we also need AVX512 compiler support (https://github.com/pytorch/FBGEMM/blob/main/CMakeLists.txt#L52). There is a recent issue reported on AMD CPU build in #1094 . Ideally we should fix this and provide the users with the flexibility on using AVX2 vs. AVX512 independent from CUDA/ROCm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why this change is needed for ROCm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROCm is on Python 3.7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I just don't understand Python, but
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
":=" is a Python 3.8 feature, see https://stackoverflow.com/a/26000366
The PyTorch upstream CI jobs for ROCm are executed with python 3.7, so all our release dockers are on 3.7. However, ROCm does not dependent on a specific python version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes maybe we should change this if FBGEMM_GPU should work with Python < 3.8,, but I am not sure about the minimum Python version for FBGEMM_GPU, but @jianyuh do you know the version requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! If the current change works for 3.7+, it looks good to me. Ideally we should use the simple syntax and avoid using the specific Python 3.8+ features.