[Bazel] Define BUILTIN_THREAD_POINTER where appropriate on x86-64 linux

[boomanaiden154]

This patch defines BUILTIN_THREAD_POINTER in config.bzl when appropriate on x86-64 linux only. This is needed to build exegesis properly as certain functionality breaks if this is not enabled. This option is only supported on relatively recent compiler versions, but given most people using the bazel build are using very recent toolchains, this shouldn't be an issue.

[boomanaiden154]

Not sure if this is the correct place to make these modifications. The comment at the end seems like it might be, but there could definitely be a better way to set this up as I'm not that familiar with Bazel. I didn't find much in the docs there about selecting specific defines based on compiler versions/other platform specific details, but it could definitely be there.

[rnk]

I think this should be done in the style of the checks in Compiler.h, somehow like this:

#if __has_builtin(__builtin_thread_pointer) || LLVM_GNUC_PREREQ(10, 0, 0)
# define HAVE_BUILTIN_THREAD_POINTER
#endif

And then secondarily, since exegesis is the only client of the define, I would actually sync it down to the one .cpp file that uses this check. We shouldn't inflict an entire configuration test compilation the entire project because one single non-essential benchmark library needs this, when a compiler version check will suffice.

[rnk]

I guess in general this does require a configure check, since GCC __has_builtin returns true for this, but then errors out during code generation:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96952#c2

We should have to replicate knowledge of which targets GCC supports this builtin on in order to forego the configure check.

[boomanaiden154]

I guess in general this does require a configure check, since GCC __has_builtin returns true for this, but then errors out during code generation: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96952#c2

Right. Originally it was a __has_builtin check, but the broke specific platforms with older gcc versions due to the linked bug.

We should have to replicate knowledge of which targets GCC supports this builtin on in order to forego the configure check.

Are you suggesting to do this through Bazel or still suggesting to sink it into the BenchmarkRunner.cpp file? I'd like to avoid changes there if possible as I believe the CMake check is a better solution than replicating that information with preprocessor macros.

Is there a way to easily do this in bazel?I saw https://github.com/llvm/llvm-project/blob/main/utils/bazel/llvm-project-overlay/llvm/config.bzl which seems like the place to put this, but I didn't find any way to select based on compiler version, which is critical to actually getting this to work (at least if we want to do it through Bazel).

[MaskRay]

The CMake check was introduced in 822c31a and adjusted (used) in a54545b. Perhaps we can also disable Bazel builds if GCC<=10 or Clang<=13 at another layer?
People who use this unofficial build system seem to use very new Clang or GCC.
(Also we don't have build bots for older compilers, so they may not even work.)

We probably don't even need to care about platforms where GCC doesn't define __builtin_thread_pointer: people don't use Bazel on these platforms.

[boomanaiden154]

The CMake check was introduced in 822c31a and adjusted (used) in a54545b. Perhaps we can also disable Bazel builds if GCC<=10 or Clang<=13 at another layer?

Didn't realize it was modified further after landing. Others would have to chime in on how to disable Bazel builds with old toolchains/whether or not that is even wanted.

People who use this unofficial build system seem to use very new Clang or GCC. (Also we don't have build bots for older compilers, so they may not even work.)

I figured most people using Bazel would be using a recent toolchain, but wasn't entirely sure and would rather be a little bit more careful, especially as the exact toolchain requirements aren't documented anywhere, so this could theoretically cause breakage on platforms that weren't explicitly marked as unsupported.

We probably don't even need to care about platforms where GCC doesn't define __builtin_thread_pointer: people don't use Bazel on these platforms.

If that's the case, then I can just add it to the Linux x86-64 defines in ./utils/bazel/llvm-project-overlay/llvm/config.bzl. I guess gcc 10 is >3 years old at this point.

[rnk]

Are you suggesting to do this through Bazel or still suggesting to sink it into the BenchmarkRunner.cpp file? I'd like to avoid changes there if possible as I believe the CMake check is a better solution than replicating that information with preprocessor macros.

I tend to take the opposite stance, configure-time checks are expensive and we pay for them on almost every build. IMO we should have a higher bar for adding new checks and we should ask ourselves what checks we can delete and replace with pre-processor conditionals. But, that's just my opinion, and I don't want to burden folks with work.

If that's the case, then I can just add it to the Linux x86-64 defines in ./utils/bazel/llvm-project-overlay/llvm/config.bzl. I guess gcc 10 is >3 years old at this point.

I like that solution.

[boomanaiden154]

I like that solution.

Thanks for working with me to find a solution. I've implemented that and added a comment explaining the details. Please let me know if there are any refactorings/style issues with how I've done it as I'm not exactly sure what the canonical Bazel way to do this would be.

[boomanaiden154]

Bump on this when reviewers have a chance. Thanks!