-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Simplify detection of device bitcode libraries for RTC #19756
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
base: sycl
Are you sure you want to change the base?
Conversation
…49294) This patch provides a single point for handling the logic behind choosing common bitcode libraries. The intention is that the users of ROCm installation detector will not have to rewrite options handling code each time the bitcode libraries are queried. This is not too distant from detectors for other architecture that encapsulate the similar decision making process, providing cleaner interface. The only flag left in `getCommonBitcodeLibs` (main point of entry) is `NeedsASanRT`, this is deliberate, as in order to calculate it we need to consult `ToolChain`.
…51114) This patch moves `LazyDetector` and target specific (Cuda, Hip, SYCL) installation detectors to clang's include directory. It was problematic for downstream to use headers from clang's lib dir. The use of lib headers could lead to subtle errors, as some of the symbols there are annotated with `LLVM_LIBRARY_VISIBILITY`. For instance [`ROCMToolChain::getCommonDeviceLibNames`](https://github.com/jchlanda/llvm-project/blob/jakub/installation_detectors/clang/lib/Driver/ToolChains/AMDGPU.h#L147) is c++ public, but because of the annotation it ends up as ELF hidden symbol, which causes errors when accessed from another shared library.
Please note that the first two commits are cherry-picks from upstream llvm: llvm/llvm-project#149294 and llvm/llvm-project#151114. |
82a4a32
to
0e0a961
Compare
0e0a961
to
912b04d
Compare
By using installation detectors we can move away from creating full toolchains and querying them.
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.
LGTM.
I focused my review on 912b04d
, as the other two commits were already reviewed in upstream LLVM.
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.
LGTM
By using installation detectors we can move away from creating full toolchains and querying them.