search

ROCm / ROCm-CompilerSupport

The compiler support repository provides various Lightning Compiler related services.
47 stars 31 forks source link

Ensure Clang/LLVM symbols are exposed for plugins #43

Closed wsmoses closed 7 months ago

wsmoses commented 2 years ago

Ideally should also be forward/backported to other relevant ROCM versions.

HIPRtc currently does not correctly support LLVM plugins. Specifically, symbols defined within LLVM and/or clang are not available by any plugins when they are loaded. This is partially due to the fact that the one location with these symbols (libamd_comgr) does not mark these symbols as being available globally when it is loaded.

This patch is the second of two pieces required to support LLVM plugins on ROCm, the first of which is a change to mark libamd_comgr as having its symbols loaded globally (https://github.com/ROCm-Developer-Tools/ROCclr/pull/32).

Specifically, successfully exporting these symbols will result in the following (note the capital T) when built:

wsmoses@noether:~/rocm/ROCm-CompilerSupport/lib/comgr/build$ nm libamd_comgr.so.2.4 | grep _ZN4llvm2cl18GenericOptionValue6anchorEv
0000000005677990 T _ZN4llvm2cl18GenericOptionValue6anchorEv
0000000005677990 t _ZN4llvm2cl18GenericOptionValue6anchorEv.localalias.28

cc @jedbrown @LeilaGhaffari

searlmc1 commented 2 years ago

Adding @b-sumner

aaronmondal commented 1 year ago

How would this affect downstream targets that also link LLVM? Our custom comgr build files here and our rocclr buildfiles here currently "blindly" link the entire LLVM.

When trying to integrate the OpenSYCL plugins into our toolchains I encountered curious issues because the new-style passmanager plugins didn't work correctly (https://github.com/OpenSYCL/OpenSYCL/issues/766). Do these two patches address such issues or am I misunderstanding something?

Linking the whole-archive llvm libs would make comgr gigantic right? Is this really necessary or would it be possible to keep only relevant plugin-related symbols from LLVM?

lamb-j commented 7 months ago

After some discussion, we decided against incorporating this PR. This would require Comgr to make symbols outside its documented API public. We can't immediately expose LLVM symbols in the Comgr DSO because users of Comgr may use other projects that expose LLVM symbols, and because it falls outside the intended role/scope of Comgr.

Some alternatives would be to upstream the desired plugin, or to do a custom build of Comgr/Rocm with this type of patch applied.