gh-Cyan4973/xxHash

Watch

Fork 0

Intel vector instruction dispatcher #387

Merged

Cyan4973 merged 21 commits from x86dispatch_table into dev

2020-06-11 22:34:32 -05:00

Cyan4973 commented

2020-06-10 18:48:28 -05:00

(Migrated from github.com)

Started from a baseline developed by @easyaspi314 (#358),
xxh_x86dispatch.c makes it possible to dynamically detect
which vector instruction set is present on the active system
and use the (presumable) best available one,
using this priority order : avx512 > avx2 > sse2 > scalar.

3rd party applications willing to employ this ability
just have to #include "xxh_x86dispatch.h".
This header automatically includes xxhash.h,
and swap relevant XXH3/XXH128 symbols
with their vector dispatch counterpart.

Only runs on x86/x64.
Only compiles with gcc and clang.

added : make dispatch target, which compiles xxhsum with this new ability.
added : make xxhsum DISPATCH=1, which is equivalent to make dispatch

Started from a baseline developed by @easyaspi314 (#358), `xxh_x86dispatch.c` makes it possible to dynamically detect which vector instruction set is present on the active system and use the (presumable) best available one, using this priority order : `avx512` > `avx2` > `sse2` > `scalar`. 3rd party applications willing to employ this ability just have to `#include "xxh_x86dispatch.h"`. This header automatically includes `xxhash.h`, and swap relevant `XXH3`/`XXH128` symbols with their vector dispatch counterpart. Only runs on `x86`/`x64`. Only compiles with `gcc` and `clang`. added : `make dispatch` target, which compiles `xxhsum` with this new ability. added : `make xxhsum DISPATCH=1`, which is equivalent to `make dispatch`

Cyan4973 commented

2020-06-10 21:42:55 -05:00

(Migrated from github.com)

It happens that my wife has one those almost rare icelake laptop, which supports avx512f.
So it's a nice playground to test this code path.

Good news : xxhsum compiles and runs fine with -mavx512f.
Bad news : dispatch compiles, with both gcc and clang, but then fails at runtime.

Exact reason is unclear yet.
gcc just immediately segfault.
clang fails during the self-test (64-bit hash test 100, likely related to xxh3).

This feels like some out-of-bound access, but need more traces.

No problem with a standard avx2 laptop or desktop, tested with macosx, linux and Windows. So, one needs the specific hardware to observe ...

Worst case, I will have to disable avx512 from the dispatcher if I can't debug it on time.

It happens that my wife has one those almost rare icelake laptop, which supports `avx512f`. So it's a nice playground to test this code path. Good news : `xxhsum` compiles and runs fine with `-mavx512f`. Bad news : `dispatch` compiles, with both `gcc` and `clang`, but then fails at runtime. Exact reason is unclear yet. `gcc` just immediately `segfault`. `clang` fails during the self-test (64-bit hash test 100, likely related to `xxh3`). This feels like some out-of-bound access, but need more traces. No problem with a standard `avx2` laptop or desktop, tested with `macosx`, `linux` and Windows. So, one needs the specific hardware to observe ... Worst case, I will have to disable `avx512` from the dispatcher if I can't debug it on time.

easyaspi314 commented

2020-06-10 22:29:36 -05:00

(Migrated from github.com)

Weird...Does my version also segfault?

(On my branch you can just run make DISPATCH=1).

Also, I believe Intel's SDE might be useful for testing AVX512.

Weird...Does my version also segfault? (On my branch you can just run `make DISPATCH=1`). Also, I believe Intel's SDE might be useful for testing AVX512.

Cyan4973 commented

2020-06-11 02:29:09 -05:00

(Migrated from github.com)

OK, this is fixed. This was an index issue, now checked with a static_assert().
Dispatcher works fine on avx512 too now.

OK, this is fixed. This was an index issue, now checked with a `static_assert()`. Dispatcher works fine on `avx512` too now.

easyaspi314 commented

2020-06-11 13:58:24 -05:00

(Migrated from github.com)

Shouldn't we use the official names for -DXXH_VECTOR? e.g. XXH_SSE2, XXH_AVX2, etc?

Shouldn't we use the official names for `-DXXH_VECTOR`? e.g. `XXH_SSE2`, `XXH_AVX2`, etc?

Cyan4973 commented

2020-06-11 14:11:15 -05:00

(Migrated from github.com)

Are you referring to the README.md documentation on XXH_VECTOR build macro ?

Are you referring to the `README.md` documentation on `XXH_VECTOR` build macro ?

easyaspi314 commented

2020-06-11 14:55:13 -05:00

(Migrated from github.com)

I mean in places like travis.yml where the bug was caused by mixing up the magic ID numbers for XXH_VECTOR.
If we just used the textual names, we wouldn't have had the issue you just had.

I mean in places like `travis.yml` where the bug was caused by mixing up the magic ID numbers for `XXH_VECTOR`. If we just used the textual names, we wouldn't have had the issue you just had.

Cyan4973 commented

2020-06-11 16:48:50 -05:00

(Migrated from github.com)

In reality, this wasn't the source of the problem. The change of vector ID numbers in .travis.yml was merely a consequence of the selected fix.

But I do nonetheless agree that expressing vector instruction set in name rather than ID would be beneficial for readability.

Thing is, I don't know how to do that without swamping the Makefile with a ton of lines of code just for this (barely used and mostly debug) feature.

Anyway, if you believe you have a good solution, you're welcomed to give it a try.
It could be part of a separate PR, as I guess it doesn't impact the current one.

In reality, this wasn't the source of the problem. The change of vector ID numbers in `.travis.yml` was merely a consequence of the selected fix. But I do nonetheless agree that expressing vector instruction set in name rather than ID would be beneficial for readability. Thing is, I don't know how to do that without swamping the `Makefile` with a ton of lines of code just for this (barely used and mostly debug) feature. Anyway, if you believe you have a good solution, you're welcomed to give it a try. It could be part of a separate PR, as I guess it doesn't impact the current one.

easyaspi314 commented

2020-06-11 22:39:22 -05:00

(Migrated from github.com)

Thing is, I don't know how to do that without swamping the Makefile with a ton of lines of code just for this (barely used and mostly debug) feature.

Huh? I am just talking about using -DXXH_VECTOR=XXH_AVX2 instead of -DXXH_VECTOR=2. It works, macros aren't expanded until they are used.

> Thing is, I don't know how to do that without swamping the Makefile with a ton of lines of code just for this (barely used and mostly debug) feature. Huh? I am just talking about using `-DXXH_VECTOR=XXH_AVX2` instead of `-DXXH_VECTOR=2`. It works, macros aren't expanded until they are used.

Cyan4973 commented

2020-06-11 23:22:02 -05:00

(Migrated from github.com)

My bad, misunderstood,
yes you are right, this is much cleaner

My bad, misunderstood, yes you are right, this is much cleaner

👍 1 😆 1

No reviewers

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Cyan4973/xxHash!387

No description provided.

Rows
Columns