Reconsider XXH_X86DISPATCH_ALLOW_AVX check

fweimer-rh commented

2023-06-19 01:39:38 -05:00

(Migrated from github.com)

Distributions are beginning to build with -march=x86-64-v3, for example Fedora ELN, so they trip over this.

This conditional #error seems to be more of a reminder to xxHash developers and not users who recompile an unchanged xxHash distribution. I think it's reasonable to assume that if the toolchain defaults to -march=x86-64-v3 (either implicitly or via injected CFLAGS), there is no assumption that the result will run on machines without AVX support.

Distributions are beginning to build with `-march=x86-64-v3`, for example Fedora ELN, so they trip over this. This conditional `#error` seems to be more of a reminder to xxHash developers and not users who recompile an unchanged xxHash distribution. I think it's reasonable to assume that if the toolchain defaults to `-march=x86-64-v3` (either implicitly or via injected `CFLAGS`), there is no assumption that the result will run on machines without AVX support.

Cyan4973 commented

2023-06-19 02:18:34 -05:00

(Migrated from github.com)

xxh_x86dispatch was designed for a scenario where the produced binary doesn't know if it will run on a cpu supporting a specific vector instruction set. Therefore, it compiles all potential variants, and then, at runtime, will select the most appropriate one, i.e. the fastest supported one on local cpu.

My understanding is that adding -mavx2 or equivalent to the command line, which is one consequence of -march=x86-64-v3, messes with this design, in the sense that it's no longer possible to generate a sse2-only variant of xxhash. The reason is, using __attribute__((__target__("xxxx"))), it's possible to add a specific vector support if it was not already present on the compilation command line, but it's not possible to remove one.

Therefore, with no additional precautions, such design would end up generating a sse2 variant which actually needs avx2. And then, at runtime, on a platform without avx2 support, it would select this variant, and crash.

So this is not great, but for people who nonetheless want to do that, presumably because they assume that the binary will never run on a platform without avx2 support, the build variable XXH_X86DISPATCH_ALLOW_AVX can be enabled, removing the restriction.

Somehow, it looks to me that this is a relatively safe default behavior. Without it, it would be relatively easy and unfortunately completely silent to produce the scenario described above, and then experience a ton of issues during deployment on a diverse set of platforms.

The build variable solution looks also fine by me, it makes the builder fully aware of the limitations of the produced binary, and requires to accept them by enabling the variable. This is very different from enabling the scenario transparently.

Now, one thing that could be improved is the error message. The way it's phrased, it just tells to not use -mavx target, but it doesn't explain why nor provide any lead towards XXH_X86DISPATCH_ALLOW_AVX (though both are explained in the source code documentation). This could be improved, so that package maintainers have an easier time discovering the build variable solution.

Now, should it become default and transparent,
that's where I am rather cautious.
For the time being, I don't think it's a good idea. It will likely make build easier, but then will possibly result in massive support headaches at deployment time. The requirement to add a build variable in order to fully acknowledge and buy in the "no-sse2-available" limitation sounds like a comparatively minor inconvenience to me.

`xxh_x86dispatch` was designed for a scenario where the produced binary doesn't know if it will run on a cpu supporting a specific vector instruction set. Therefore, it compiles all potential variants, and then, at runtime, will select the most appropriate one, i.e. the fastest supported one on local cpu. My understanding is that adding `-mavx2` or equivalent to the command line, which is one consequence of `-march=x86-64-v3`, messes with this design, in the sense that it's no longer possible to generate a `sse2`-only variant of `xxhash`. The reason is, using `__attribute__((__target__("xxxx")))`, it's possible to _add_ a specific vector support if it was not already present on the compilation command line, but it's not possible to _remove_ one. Therefore, with no additional precautions, such design would end up generating a `sse2` variant which actually needs `avx2`. And then, at runtime, on a platform without `avx2` support, it would select this variant, and crash. So this is not great, but for people who nonetheless want to do that, presumably because they assume that the binary will never run on a platform without `avx2` support, the build variable `XXH_X86DISPATCH_ALLOW_AVX` can be enabled, removing the restriction. Somehow, it looks to me that this is a relatively safe default behavior. Without it, it would be relatively easy and unfortunately completely silent to produce the scenario described above, and then experience a ton of issues during deployment on a diverse set of platforms. The build variable solution looks also fine by me, it makes the builder fully aware of the limitations of the produced binary, and requires to accept them by enabling the variable. This is very different from enabling the scenario transparently. Now, one thing that could be improved is the error message. The way it's phrased, it just tells to _not_ use `-mavx` target, but it doesn't explain why nor provide any lead towards `XXH_X86DISPATCH_ALLOW_AVX` (though both are explained in the source code documentation). This could be improved, so that package maintainers have an easier time discovering the build variable solution. Now, should it become default and transparent, that's where I am rather cautious. For the time being, I don't think it's a good idea. It will likely make build easier, but then will possibly result in massive support headaches at deployment time. The requirement to add a build variable in order to fully acknowledge and buy in the "no-sse2-available" limitation sounds like a comparatively minor inconvenience to me.

fweimer-rh commented

2023-06-19 02:46:07 -05:00

(Migrated from github.com)

Somehow, it looks to me that this is a relatively safe default behavior. Without it, it would be relatively easy and unfortunately completely silent to produce the scenario described above, and then experience a ton of issues during deployment on a diverse set of platforms.

But what makes xxHash special that it needs this treatment? There are hundreds of other packages in a typical distribution, some with run-time dispatch, some without, and they don't do have #errors like xxHash. If people copy incompatible binaries from machine to machine, they will likely encounter problems elsewhere.

I agree that good diagnostics are important, but ideally, we want a run-time diagnostic if a binary is executed on an incompatible machine. We have implemented that in glibc, but so far it is only effective for glibc itself because it is difficult to generate the appropriate markers automatically in the link editor (because it does not know if the AVX code linked into the output file is executed conditionally or not).

> Somehow, it looks to me that this is a relatively safe default behavior. Without it, it would be relatively easy and unfortunately completely silent to produce the scenario described above, and then experience a ton of issues during deployment on a diverse set of platforms. But what makes xxHash special that it needs this treatment? There are hundreds of other packages in a typical distribution, some with run-time dispatch, some without, and they don't do have `#error`s like xxHash. If people copy incompatible binaries from machine to machine, they will likely encounter problems elsewhere. I agree that good diagnostics are important, but ideally, we want a run-time diagnostic if a binary is executed on an incompatible machine. We have implemented that in glibc, but so far it is only effective for glibc itself because it is difficult to generate the appropriate markers automatically in the link editor (because it does not know if the AVX code linked into the output file is executed conditionally or not).

Cyan4973 commented

2023-06-19 03:00:29 -05:00

(Migrated from github.com)

libxxhash isn't supposed to use nor link to stdio.
There's no way to generate any message of any kind from the library at run time.

Printing diagnostic messages could be done from the CLI, though that is a completely different target.
Issue is, xxh_x86dispatch is not tied to the CLI, it's designed to be integrated at both library and CLI levels.

Another thing I'm not sure to understand is why it is such a problem to add a compilation variable ?
The default build adds xxhash only, and this unit conforms to the set of flags provided at compilation time,
meaning it will have no problem compiling with -march=x86-64-v3, and will use AVX2 mode in this case.
Inserting xxh_x86dispatch is not a default operation, and ironically requires setting a build variable. Hence, what's the problem with adding another one ?

`libxxhash` isn't supposed to use nor link to `stdio`. There's no way to generate any message of any kind from the library at run time. Printing diagnostic messages could be done from the CLI, though that is a completely different target. Issue is, `xxh_x86dispatch` is not tied to the CLI, it's designed to be integrated at both library and CLI levels. Another thing I'm not sure to understand is why it is such a problem to add a compilation variable ? The default build adds `xxhash` only, and this unit conforms to the set of flags provided at compilation time, meaning it will have no problem compiling with `-march=x86-64-v3`, and will use `AVX2` mode in this case. Inserting `xxh_x86dispatch` is not a default operation, and ironically requires setting a build variable. Hence, what's the problem with adding another one ?

Cyan4973 commented

2023-07-02 19:04:02 -05:00

(Migrated from github.com)

Summarizing the position expressed in this issue :

If a build config enables AVX2 extension, then it accepts that the produced binary will not work on a platform without AVX2 support.
Therefore, it's not useful to produce an auto-vectorization library which supports SSE2 only.

This is understandable.
Unfortunately, xxh_x86dispatch wasn't created with this scenario in mind. So, instead, it simply required that this scenario doesn't exist.

Proper adaptation to this new requirement would require an ability to bypass SSE2 code path generation (currently it's always produced), and be paired with an ability to provide a clear error message for platforms without AVX2 support (instead of a segfault), which can only be done in the CLI code.

In the meantime, the current work around which simply removes the "no avx2" protection and let the library be built with the AVX2 capability flag will do. It will "work" fine on compatible systems. It's just that it will nonetheless produce a (now useless) SSE2 code path, and will segfault on systems without AVX2 support.

Summarizing the position expressed in this issue : If a build config enables `AVX2` extension, then it accepts that the produced binary will not work on a platform without `AVX2` support. Therefore, it's not useful to produce an auto-vectorization library which supports `SSE2` only. This is understandable. Unfortunately, `xxh_x86dispatch` wasn't created with this scenario in mind. So, instead, it simply required that this scenario doesn't exist. Proper adaptation to this new requirement would require an ability to bypass `SSE2` code path generation (currently it's always produced), and be paired with an ability to provide a clear error message for platforms without `AVX2` support (instead of a segfault), which can only be done in the CLI code. In the meantime, the current work around which simply removes the "no avx2" protection and let the library be built with the `AVX2` capability flag will do. It will "work" fine on compatible systems. It's just that it will nonetheless produce a (now useless) `SSE2` code path, and will segfault on systems without AVX2 support.

Cyan4973 commented

2024-12-08 17:40:19 -06:00

(Migrated from github.com)

Looking back at this issue, it inspires some comments :

XXH_X86DISPATCH_ALLOW_AVX doesn't feel correct, as a build macro name. Rather, I believe it should have been XXH_X86DISPATCH_REQUIRE_AVX2, aka the produced binary will fail if not run on a system supporting at least AVX2.
I understand that it would be even more convenient to not have to set a compilation variable at all. The counter point is that it would be easy to silently generate a DISPATCH variant which doesn't support SSE2, and would then crash on systems without AVX2.
Speaking of which, one issue is that running such a variant on a system without AVX2 support would result in a crash. That's undesirable. A better outcome would be a clean exit. Problem, this is not possible at xxh_x86dispatch.c level, since stdio is not present there, and some functions are not even allowed to fail. But it would be possible at CLI level. The consequence though would be some additional complexity to support this scenario in the CLI and test framework, and its ongoing maintenance. Without specifics, it's not clear at this point if this is a desirable trade off.

Looking back at this issue, it inspires some comments : - `XXH_X86DISPATCH_ALLOW_AVX` doesn't feel correct, as a build macro name. Rather, I believe it should have been `XXH_X86DISPATCH_REQUIRE_AVX2`, aka the produced binary will fail if not run on a system supporting at least `AVX2`. - I understand that it would be even more convenient to not have to set a compilation variable at all. The counter point is that it would be easy to silently generate a `DISPATCH` variant which doesn't support `SSE2`, and would then crash on systems without `AVX2`. - Speaking of which, one issue is that running such a variant on a system without `AVX2` support would result in a crash. That's undesirable. A better outcome would be a clean exit. Problem, this is not possible at `xxh_x86dispatch.c` level, since `stdio` is not present there, and some functions are not even allowed to fail. But it would be possible at CLI level. The consequence though would be some additional complexity to support this scenario in the CLI and test framework, and its ongoing maintenance. Without specifics, it's not clear at this point if this is a desirable trade off.

Cyan4973 commented

2024-12-26 18:50:36 -06:00

(Migrated from github.com)

The next release of xxhash (v0.8.3) will use a different policy, where DISPATCH=1 will be automatically enabled when target system is x86/x64, and compilation will work whatever is the minimum instruction set required, without the need to employ a specific build variable.
Of course, in such a case, the produced binary can only work on systems featuring the requested instruction set.

The next release of `xxhash` (`v0.8.3`) will use a different policy, where `DISPATCH=1` will be automatically enabled when target system is `x86`/`x64`, and compilation will work whatever is the minimum instruction set required, without the need to employ a specific build variable. Of course, in such a case, the produced binary can only work on systems featuring the requested instruction set.

Rows
Columns

Reconsider XXH_X86DISPATCH_ALLOW_AVX check #839