[PATCH] D143768: [Clang] Add options to disable direct linking of arch tools

Mon Feb 13 09:00:25 PST 2023

aaronmondal added a subscriber: aaron.ballman.
aaronmondal added a comment.

Ok so I went over https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations#testing-for-the-presence-of-a-header-__has_include (thanks, @aaron.ballman 😊) and through the commit history for the amdgpu-arch tool.

As I understand, in cases such as standard library headers, these `__has_include`s are useful (and are encouraged), (I assume especially for header-only libraries). But I'm not convinced that it's the right way to do things in the HSA case.

1. If we don't detect this during configuration, we will fail during the build, not during configuration. this means that if the hsa CMake package is found but the headers are not actually present, we will fail late, potentially *very* late in the build.
2. It also seems like the `__has_include`s have continuously been a source of confusion, and maybe I'm confused as well, but I think even the current implementation after https://reviews.llvm.org/rGbfe4514add5b7ab7e1f06248983a7162d734cffb has a bug (if DYNAMIC_HSA is not set and hsa is not found, we don't raise a "missing hsa.h" error).

@jhuber6 I didn't understand what you mean with linking it into libc. But if the performance regression is "ignorable" I'd be in favor of just always using `dlopen`.

I also agree with @tianshilei1992 that if this should remain we should definitely have configuration for this.

IMO it would be very desirable for every out-of-llvm-tree dependency to be checked during configuration. If someone without access to a compile server runs an LLVM build that seemingly passes configuration and then fails after 90 minutes because they had leftover cmake configs their package manager failed to clean up it unnecessarily inconveniences the user and we can prevent it.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143768/new/

https://reviews.llvm.org/D143768