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

Simplify ROCM bitcode libraries to use compiler-provided flags. #15148

Open
benvanik opened this issue Oct 10, 2023 · 5 comments
Open

Simplify ROCM bitcode libraries to use compiler-provided flags. #15148

benvanik opened this issue Oct 10, 2023 · 5 comments
Assignees
Labels
codegen Shared code generation infrastructure and dialects

Comments

@benvanik
Copy link
Collaborator

The files under third_party/ROCm-Device-Libs/oclc/ are all pure configuration; see
https://github.com/RadeonOpenCompute/ROCm-Device-Libs/blob/amd-stg-open/oclc/inc/oclc.h#L44-L51

These external constant values are used by other larger parts of the code to specialize at compile/link time, and the .cl files under oclc/ are all just setting specific values as a way to (probably) make it easier to compile things from the command line. So linking in third_party/ROCm-Device-Libs/oclc/src/isa_version_601.cl (compiled to bc) literally just sets const __constant int __oclc_ISA_version = 6001;, and isa_version_602.cl sets it to 6002. Same with flags for rounding modes and such.

Instead we can set these programmatically in the compiler as we do for CUDA/CPU:
https://github.com/openxla/iree/blob/main/compiler/src/iree/compiler/Dialect/HAL/Target/LLVMCPU/Builtins/Device.cpp#L85-L102

So ROCMTarget.cpp could read the target configuration and then use something like overridePlatformGlobal("__oclc_ISA_version", 6001); instead of linking in isa_version_601.bc. This means we don't need any of the oclc/ files and only need the actual libdevice code like third_party/ROCm-Device-Libs/ocml/src/acospiH.cl (or whatever). This code is all target/config agnostic and a single .bc we produced with all of that (like opencl.bc) would work with any of the configuration we set via the globals.

This should let us simplify things to be just like the CUDA side: we set the reflection information (__oclc_...) programmatically, ship a .bc of all the generic implementation bits, and let LLVM propagate the values for us. We would no longer need to build from source as the same .bc would work with all configurations a user may target with the compiler and thus we can vendor it out like we do libdevice.bc in CUDA.

@benvanik benvanik added the codegen Shared code generation infrastructure and dialects label Oct 10, 2023
@powderluv
Copy link
Collaborator

this is awesome.

@raikonenfnu
Copy link
Collaborator

@benvanik I have a draft implementation of this here raikonenfnu@707bce0

This is currently not working though, because the globalVariables doesn't exist in llvmModule. Do you have any thoughts on doing either:

  • Creating a new globalVariable for every one of these params and inserting them into the llvmModule manually
  • Somehow plumb the global variables at a higher level. (not really sure how to do this though)

Happy to hear your thoughts/notes on the above implementations. :)

@benvanik
Copy link
Collaborator Author

So speedy! I'd just create them if not found.

@raikonenfnu
Copy link
Collaborator

@benvanik Thanks for the guidance Ben! Was playing out/matching the GlobalValue params to match as much as possible and work on the models, pushed up a PR. :)

@raikonenfnu
Copy link
Collaborator

raikonenfnu commented Oct 16, 2023

@benvanik I moved the linking with global after linking with BC. This would let linking with BC to introduce required globalValues. So technically we just need to override the initializer. I tested and it works/runs llama. :)

https://github.com/openxla/iree/pull/15169/files#diff-f298bbd07fa562105b7cd6dae43fecff063f9aef80b864de2800fcecb8e644ac

raikonenfnu added a commit that referenced this issue Oct 16, 2023
…15169)

Linking simple parameters through globalVariables rather than bc.
Based on the concept introduced in
#15148
ramiro050 pushed a commit to ramiro050/iree that referenced this issue Dec 19, 2023
…ree-org#15169)

Linking simple parameters through globalVariables rather than bc.
Based on the concept introduced in
iree-org#15148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Shared code generation infrastructure and dialects
Projects
None yet
Development

No branches or pull requests

4 participants