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

[ROCM] Add Thrust support #7458

Merged
merged 10 commits into from
Feb 17, 2021
Merged

[ROCM] Add Thrust support #7458

merged 10 commits into from
Feb 17, 2021

Conversation

masahi
Copy link
Member

@masahi masahi commented Feb 15, 2021

This adds rocThrust support for our rocm backend, which can be used for sorting and scan.
https://github.com/ROCmSoftwarePlatform/rocThrust

Use "rocm -libs=thrust" to enable thrust based sort and scan.

contrib/rocthrust/thrust.cc code is identical to contrib/thrust/thrust.cu used by CUDA backend. The same thrust code works on both CUDA and AMD.

@mbrookhart @csullivan @antinucleon

@masahi
Copy link
Member Author

masahi commented Feb 16, 2021

@tkonolige @mbrookhart Thanks for review, it should be ready to go. This can be used for benchmarking the new TIR sort @mbrookhart is cooking.

I'll follow up with another PR to enforce -libs=thrust option for other thrust usages.

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

Devil's advocate:

If I reformat the thrust.cu file with clang-format-10, like you said, they are identical except for the datatype errors you added and #include <thrust/sequence.h> in the amd version.

Is there any reason to keep two copies around?

(otherwise LGTM, thank you!)

Copy link
Contributor

@csullivan csullivan left a comment

Choose a reason for hiding this comment

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

Thanks @masahi for adding rocthrust support!

python/tvm/relay/op/strategy/rocm.py Show resolved Hide resolved
@masahi
Copy link
Member Author

masahi commented Feb 16, 2021

@mbrookhart Yes, I wanted to reuse thrust.cu as is, by adding the following to ROCM.cmake:

file(GLOB CONTRIB_THRUST_SRC src/runtime/contrib/thrust/*.cu)

But unlike for CUDA, cmake doesn't recognize that this .cu file needs to be compiled by the same CXX compiler (hipcc) together with other .cc files, so it will not be compiled (it is ignored during compilation).

I think CMake has a special support for NVCC and .cu file, to correctly compile .cu with NVCC and others with gcc/clang. On rocm, everything needs to be compiled by the same compiler, hipcc (a wrapper around clang).

@masahi
Copy link
Member Author

masahi commented Feb 16, 2021

Maybe pytorch people have a solution to the duplication problem. But my cmake-fu is too low and I have no interest in learning about cmake :)

@mbrookhart
Copy link
Contributor

@tkonolige @csullivan Any other change requests or are you happy?

@tkonolige
Copy link
Contributor

@masahi You can use set_source_files_properties(src/runtime/contrib/thrust/thrust.cu PROPERTIES LANGUAGE CXX).

@masahi
Copy link
Member Author

masahi commented Feb 16, 2021

@masahi You can use set_source_files_properties(src/runtime/contrib/thrust/thrust.cu PROPERTIES LANGUAGE CXX).

Thanks I'll try this ASAP

@masahi
Copy link
Member Author

masahi commented Feb 17, 2021

Great set_source_files_properties worked! I removed rocthrust/thrust.cc, the same thrust.cu is used by CUDA and rocm now cc @mbrookhart

@masahi masahi merged commit e57e644 into apache:main Feb 17, 2021
@masahi
Copy link
Member Author

masahi commented Feb 17, 2021

Thanks @mbrookhart @tkonolige @csullivan

Lokiiiiii pushed a commit to Lokiiiiii/tvm that referenced this pull request Mar 2, 2021
* enable rocm thrust, confrimed to work on sort and scan

* add rocm argsort strategy

* Abort if CXX is not hipcc

* add more strategy

* add missing import

* fix lint

* show supported data type in err msg

* try remove rocthrust

* add missing include for rocthrust

* more minor change

Co-authored-by: Masahiro Masuda <masahi@129@gmail.com>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2021
* enable rocm thrust, confrimed to work on sort and scan

* add rocm argsort strategy

* Abort if CXX is not hipcc

* add more strategy

* add missing import

* fix lint

* show supported data type in err msg

* try remove rocthrust

* add missing include for rocthrust

* more minor change

Co-authored-by: Masahiro Masuda <masahi@129@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants