gh-Cyan4973/xxHash
Watch
1
0
Fork
You've already forked xxHash
 0
Code Issues 7 Pull requests 3 Releases 26 Wiki Activity

[WIP] implementation of XXH3 dispatcher #358

Closed
easyaspi314 wants to merge 3 commits from dispatcher-xxh3 into dev
pull from: dispatcher-xxh3
merge into: gh-Cyan4973:dev
Conversation 51 Commits 3 Files changed 9 +892 -24
easyaspi314 commented 2020-05-07 13:35:39 -05:00 (Migrated from github.com)

Very rough and untested implementation for now. Might upload binaries later.

The following functions should now dispatch on x86/x86_64:

  XXH3_hashLong_64b_defaultSecret
  XXH3_hashLong_64b_withSeed
  XXH3_hashLong_64b_withSecret
  XXH3_64bits_update (via new wrapper function)
  XXH3_hashLong_128b_defaultSecret
  XXH3_hashLong_128b_withSeed
  XXH3_hashLong_128b_withSecret
  XXH3_128bits_update (via new wrapper function)

Short hashes are not affected by this change, to prevent slowdowns when only working on short strings.

Makefile needs to be updated, for now it is compiled like so:

  gcc -fPIC -O3 -c xxhash.c -o xxhash.o -DXXH3_DISPATCH -march=x86-64
  gcc -fPIC -O3 -c xxhash.c -o xxhash-scalar.o -DXXH3_DISPATCH \
     -DXXH_TARGET=XXH_SCALAR -march=x86-64
  gcc -fPIC -O3 -c xxhash.c -o xxhash-sse2.o -DXXH3_DISPATCH \
     -DXXH_TARGET=XXH_SSE2 -march=x86-64 -msse2
  gcc -fPIC -O3 -c xxhash.c -o xxhash-avx2.o -DXXH3_DISPATCH \
     -DXXH_TARGET=XXH_AVX2 -march=x86-64 -mtune=haswell -mavx2
  gcc -fPIC -O3 -c xxhash.c -o xxhash-avx512.o -DXXH3_DISPATCH \
     -DXXH_TARGET=XXH_AVX512 -march=x86-64 -mtune=knl -mavx512f
  gcc -fPIC -O3 -c xxh3-dispatch.c -o xxh3-dispatch.o \
     -DXXH3_DISPATCH_AVX2 -DXXH3_DISPATCH_AVX512 -march=x86-64
  gcc -fPIC -O3 -c xxhsum.c -o xxhsum.o -march=x86-64
  gcc -fPIC -O3 xxhash.o xxhash-scalar.o xxhash-sse2.o xxhash-avx2.o \
     xxhash-avx512.o xxh3-dispatch.o xxhsum.o -o xxhsum
  • Mostly untested
  • Should remove xxhash-scalar.o and put the scalar implementation in
    xxhash.o
  • Simplify/optimize CPU checks
  • Simplify build system
  • Clean things up a bit
  • Document what we are doing
  • Add XXH_NAMESPACE support
Very rough and untested implementation for now. Might upload binaries later. The following functions should now dispatch on x86/x86_64: ``` XXH3_hashLong_64b_defaultSecret XXH3_hashLong_64b_withSeed XXH3_hashLong_64b_withSecret XXH3_64bits_update (via new wrapper function) XXH3_hashLong_128b_defaultSecret XXH3_hashLong_128b_withSeed XXH3_hashLong_128b_withSecret XXH3_128bits_update (via new wrapper function) ``` Short hashes are not affected by this change, to prevent slowdowns when only working on short strings. Makefile needs to be updated, for now it is compiled like so: ``` gcc -fPIC -O3 -c xxhash.c -o xxhash.o -DXXH3_DISPATCH -march=x86-64 gcc -fPIC -O3 -c xxhash.c -o xxhash-scalar.o -DXXH3_DISPATCH \ -DXXH_TARGET=XXH_SCALAR -march=x86-64 gcc -fPIC -O3 -c xxhash.c -o xxhash-sse2.o -DXXH3_DISPATCH \ -DXXH_TARGET=XXH_SSE2 -march=x86-64 -msse2 gcc -fPIC -O3 -c xxhash.c -o xxhash-avx2.o -DXXH3_DISPATCH \ -DXXH_TARGET=XXH_AVX2 -march=x86-64 -mtune=haswell -mavx2 gcc -fPIC -O3 -c xxhash.c -o xxhash-avx512.o -DXXH3_DISPATCH \ -DXXH_TARGET=XXH_AVX512 -march=x86-64 -mtune=knl -mavx512f gcc -fPIC -O3 -c xxh3-dispatch.c -o xxh3-dispatch.o \ -DXXH3_DISPATCH_AVX2 -DXXH3_DISPATCH_AVX512 -march=x86-64 gcc -fPIC -O3 -c xxhsum.c -o xxhsum.o -march=x86-64 gcc -fPIC -O3 xxhash.o xxhash-scalar.o xxhash-sse2.o xxhash-avx2.o \ xxhash-avx512.o xxh3-dispatch.o xxhsum.o -o xxhsum ``` - Mostly untested - Should remove xxhash-scalar.o and put the scalar implementation in xxhash.o - Simplify/optimize CPU checks - Simplify build system - Clean things up a bit - Document what we are doing - Add `XXH_NAMESPACE` support
👍 1
easyaspi314 commented 2020-05-07 13:40:01 -05:00 (Migrated from github.com)

It will print what features it has detected to stdout for the testing period, looking like this:

Detected SSE2
Detected AVX2
Detected AVX512F
It will print what features it has detected to stdout for the testing period, looking like this: ``` Detected SSE2 Detected AVX2 Detected AVX512F ```
👍 1
easyaspi314 commented 2020-05-07 13:42:59 -05:00 (Migrated from github.com)

The way it currently works is that it will compile the target objects with XXH_INLINE_ALL, therefore nopping the rest of the xxHash implementation, and emit a single symbol:

XXH3_dispatcher_table_<numeric value of XXH_TARGET>

The dispatch table is abstracted away with a macro:

XXH3_DISPATCH_CALL(name_without_"XXH3_")(args);

so for example, XXH3_hashLong_64b_defaultSecret is called as

XXH3_DISPATCH_CALL(hashLong_64b_defaultSecret)(...);

This will either expand to

XXH3_hashLong_64b_defaultSecret(...);

Or

XXH3_dispatcher_table->hashLong_64b_defaultSecret(...);
The way it currently works is that it will compile the target objects with XXH_INLINE_ALL, therefore nopping the rest of the xxHash implementation, and emit a single symbol: `XXH3_dispatcher_table_<numeric value of XXH_TARGET>` The dispatch table is abstracted away with a macro: ```c XXH3_DISPATCH_CALL(name_without_"XXH3_")(args); ``` so for example, `XXH3_hashLong_64b_defaultSecret` is called as ```c XXH3_DISPATCH_CALL(hashLong_64b_defaultSecret)(...); ``` This will either expand to ```c XXH3_hashLong_64b_defaultSecret(...); ``` Or ```c XXH3_dispatcher_table->hashLong_64b_defaultSecret(...); ```
easyaspi314 commented 2020-05-07 18:55:59 -05:00 (Migrated from github.com)

Noticed that I wasn't dispatching some of XXH128's functions.

On Windows, it successfully detects AVX2 on my Ryzen.

Also, using Intel's SDE program, I tested a 32-bit version targeting the original Pentium, and aside from complaining about a cmovz in the CRT startup routine, it seems to work perfectly fine on the scalar path. It also seems to detect AVX512 when I target it.

I'm going to test disabling XSAVE on my PC and see if it switches to SSE2, then I'll upload some test binaries.

Noticed that I wasn't dispatching some of XXH128's functions. On Windows, it successfully detects AVX2 on my Ryzen. Also, using Intel's SDE program, I tested a 32-bit version targeting the original Pentium, and aside from complaining about a `cmovz` in the CRT startup routine, it seems to work perfectly fine on the scalar path. It also seems to detect AVX512 when I target it. I'm going to test disabling XSAVE on my PC and see if it switches to SSE2, then I'll upload some test binaries.
👍 1
easyaspi314 commented 2020-05-07 19:23:38 -05:00 (Migrated from github.com)

With

bcdedit /set xsavedisable 1

it successfully selects the SSE2 codepath.

Here are the binaries and the shell/batch scripts for building it.

These include Windows x86 and x86_64 binaries generated by GCC 9.3, Clang 10.0, and MSVC 2019, as well as static Linux x86_64 binaries compiled by Clang 6.0.0 and GCC 7.4.0 (yay Ubuntu 18.04 LTS).

xxhash-dispatch.zip

With ``` bcdedit /set xsavedisable 1 ``` it successfully selects the SSE2 codepath. Here are the binaries and the shell/batch scripts for building it. These include Windows x86 and x86_64 binaries generated by GCC 9.3, Clang 10.0, and MSVC 2019, as well as static Linux x86_64 binaries compiled by Clang 6.0.0 and GCC 7.4.0 (yay Ubuntu 18.04 LTS). [xxhash-dispatch.zip](https://github.com/Cyan4973/xxHash/files/4596375/xxhash-dispatch.zip)
👍 1
Cyan4973 commented 2020-05-09 12:37:43 -05:00 (Migrated from github.com)

Looking into this PR,
I see that the selected methodology is mostly build driven :
it requires xxhash.c to be compiled several times with different intrinsic code paths.

I suspect it will be difficult to integrate by 3rd-party applications.

I was wondering if integration could be simplified by avoiding the difficult build stage.
For example, xxhash.h could be included multiple times within xxh3-dispatch.c using different code path selection.

For this to be possible, I presume it would require a combination of XXH_INLINE_ALL with XXH_NAMESPACE, so that each variant receives a different name. The dispatch can then select the proper variant at runtime.

If that's a direction that sounds correct, I could put together a PR which allows combining XXH_INLINE_ALL with XXH_NAMESPACE, a capability that's not present yet.

Looking into this PR, I see that the selected methodology is mostly build driven : it requires `xxhash.c` to be compiled several times with different intrinsic code paths. I suspect it will be difficult to integrate by 3rd-party applications. I was wondering if integration could be simplified by avoiding the difficult build stage. For example, `xxhash.h` could be included multiple times within ` xxh3-dispatch.c` using different code path selection. For this to be possible, I presume it would require a combination of `XXH_INLINE_ALL` with `XXH_NAMESPACE`, so that each variant receives a different name. The dispatch can then select the proper variant at runtime. If that's a direction that sounds correct, I could put together a PR which allows combining `XXH_INLINE_ALL` with `XXH_NAMESPACE`, a capability that's not present yet.
easyaspi314 commented 2020-05-09 13:37:23 -05:00 (Migrated from github.com)

Well we could do what HighwayHash did, using stub "dummy" files:

https://github.com/google/highwayhash/blob/master/highwayhash/hh_sse41.cc

My reasoning for breaking off at hashLong is to avoid the overhead of dispatching on short hashes on single shot. Most of the time you will be using it for a hash table, and that might never call hashLong.

Theoretically, we could XXH_INLINE_ALL the primary implementation, and dispatch off from there. Haven't figured out the best way to do it, though.

Well we could do what HighwayHash did, using stub "dummy" files: https://github.com/google/highwayhash/blob/master/highwayhash/hh_sse41.cc My reasoning for breaking off at `hashLong` is to avoid the overhead of dispatching on short hashes on single shot. Most of the time you will be using it for a hash table, and that might never call `hashLong`. Theoretically, we could `XXH_INLINE_ALL` the primary implementation, and dispatch off from there. Haven't figured out the best way to do it, though.
👍 1
Cyan4973 commented 2020-05-09 13:41:08 -05:00 (Migrated from github.com)

My reasoning for breaking off at hashLong is to avoid the overhead of dispatching on short hashes on single shot.

I think that's a good point. Dispatcher should only be triggered for long inputs.

> My reasoning for breaking off at hashLong is to avoid the overhead of dispatching on short hashes on single shot. I think that's a good point. Dispatcher should only be triggered for long inputs.
easyaspi314 commented 2020-05-10 17:52:22 -05:00 (Migrated from github.com)

This Makefile logic will detect the supported flags for GCC and Clang.

On GCC 4.2.4, it only compiles SSE2 files, as it doesn't support AVX2 or AVX512.

CFLAGS ?= -O3 -g -Wall

# Don't do feature testing on make clean
ifneq (clean,$(filter clean,$(MAKECMDGOALS)))
  # Makefile custom function
  test_macros = $(shell $(CC) $(CFLAGS) $(CPPFLAGS) $(1) -E -dM -xc /dev/null 2>/dev/null || echo error)
  DEFAULT_MACROS = $(call test_macros)

  # Figure out whether we are compiling for x86 or x86_64, and use the correct
  # -march flag
  ifneq (,$(filter __amd64__ _M_X64 __x86_64__,$(DEFAULT_MACROS)))
    CFLAGS += -march=x86-64
  else
    ifeq (,$(filter _M_IX86 __i686__ __i386__,$(DEFAULT_MACROS)))
      $(error Dispatching is not supported on this target!)
      ERROR_DISPATCHING_NOT_SUPPORTED
    else # i386
      CFLAGS += -march=i386
      # Try to add a recent -mtune flag to prevent GCC from making aggressive
      # shift+add optimizations.
      ifneq (error,$(call test_macros,-Werror -mtune=haswell))
        CFLAGS += -mtune=haswell
      else
        ifneq (error,$(call test_macros,-Werror -mtune=opteron))
          CFLAGS += -mtune=opteron
        endif # -mtune=opteron
      endif # -mtune=haswell
    endif # i386
  endif # x86_64

  SOURCES := xxh3-dispatch.c xxhash-sse2.c
  xxhash-sse2.o: CFLAGS += -msse2

  # Check for AVX2 support
  ifneq (,$(filter __AVX2__,$(call test_macros,-mavx2)))
    SOURCES += xxhash-avx2.c
    AVX2_FLAG = -mavx2
    # Try to add the -mno-avx256-split-unaligned-load flag on GCC to prevent load
    # splitting, an optimization for Ivy/Sandy which don't even support AVX2.
    ifneq (error,$(call test_macros,-mavx2 -mno-avx256-split-unaligned-load))
      AVX2_FLAG += -mno-avx256-split-unaligned-load
    endif
    xxhash-avx2.o: CFLAGS += $(AVX2_FLAG)
    CPPFLAGS += -DXXH3_DISPATCH_AVX2
  endif # avx2

  ifneq (,$(filter __AVX512F__,$(call test_macros,-mavx512f)))
    SOURCES += xxhash-avx512.c
    xxhash-avx512.o: CFLAGS += -mavx512f
    CPPFLAGS += -DXXH3_DISPATCH_AVX512
  endif # avx512
endif # not make clean
OBJS := $(SOURCES:.c=.o) xxhsum.o
HEADERS := ../xxhash.h ../xxh3.h

xxhsum: $(OBJS)
	$(CC) $(CFLAGS) $(LDFLAGS) $^ -o xxhsum

%.o: %.c ../xxhash.h ../xxh3.h
	$(CC) $(CFLAGS) $(CPPFLAGS) -c $< -o $@

xxhsum.o: ../xxhsum.c ../xxhash.h ../xxh3.h
	$(CC) $(CFLAGS) $(CPPFLAGS) -c $< -o $@

clean:
	$(RM) *.o xxhsum xxhsum.exe

And the target clones can be implemented like this:

/*
 * A dummy file to make dispatching easier for the build system.
 */

#define XXH_STATIC_LINKING_ONLY   /* access advanced declarations */
#define XXH_IMPLEMENTATION        /* access definitions */
#define XXH3_DISPATCH             /* Mark a dispatcher target */
#define XXH_VECTOR XXH_SSE2       /* Set target */
#define XXH_INLINE_ALL            /* Disable other symbols, only emit a table */


#include "../xxhash.h"
This Makefile logic will detect the supported flags for GCC and Clang. On GCC 4.2.4, it only compiles SSE2 files, as it doesn't support AVX2 or AVX512. ```makefile CFLAGS ?= -O3 -g -Wall # Don't do feature testing on make clean ifneq (clean,$(filter clean,$(MAKECMDGOALS))) # Makefile custom function test_macros = $(shell $(CC) $(CFLAGS) $(CPPFLAGS) $(1) -E -dM -xc /dev/null 2>/dev/null || echo error) DEFAULT_MACROS = $(call test_macros) # Figure out whether we are compiling for x86 or x86_64, and use the correct # -march flag ifneq (,$(filter __amd64__ _M_X64 __x86_64__,$(DEFAULT_MACROS))) CFLAGS += -march=x86-64 else ifeq (,$(filter _M_IX86 __i686__ __i386__,$(DEFAULT_MACROS))) $(error Dispatching is not supported on this target!) ERROR_DISPATCHING_NOT_SUPPORTED else # i386 CFLAGS += -march=i386 # Try to add a recent -mtune flag to prevent GCC from making aggressive # shift+add optimizations. ifneq (error,$(call test_macros,-Werror -mtune=haswell)) CFLAGS += -mtune=haswell else ifneq (error,$(call test_macros,-Werror -mtune=opteron)) CFLAGS += -mtune=opteron endif # -mtune=opteron endif # -mtune=haswell endif # i386 endif # x86_64 SOURCES := xxh3-dispatch.c xxhash-sse2.c xxhash-sse2.o: CFLAGS += -msse2 # Check for AVX2 support ifneq (,$(filter __AVX2__,$(call test_macros,-mavx2))) SOURCES += xxhash-avx2.c AVX2_FLAG = -mavx2 # Try to add the -mno-avx256-split-unaligned-load flag on GCC to prevent load # splitting, an optimization for Ivy/Sandy which don't even support AVX2. ifneq (error,$(call test_macros,-mavx2 -mno-avx256-split-unaligned-load)) AVX2_FLAG += -mno-avx256-split-unaligned-load endif xxhash-avx2.o: CFLAGS += $(AVX2_FLAG) CPPFLAGS += -DXXH3_DISPATCH_AVX2 endif # avx2 ifneq (,$(filter __AVX512F__,$(call test_macros,-mavx512f))) SOURCES += xxhash-avx512.c xxhash-avx512.o: CFLAGS += -mavx512f CPPFLAGS += -DXXH3_DISPATCH_AVX512 endif # avx512 endif # not make clean OBJS := $(SOURCES:.c=.o) xxhsum.o HEADERS := ../xxhash.h ../xxh3.h xxhsum: $(OBJS) $(CC) $(CFLAGS) $(LDFLAGS) $^ -o xxhsum %.o: %.c ../xxhash.h ../xxh3.h $(CC) $(CFLAGS) $(CPPFLAGS) -c $< -o $@ xxhsum.o: ../xxhsum.c ../xxhash.h ../xxh3.h $(CC) $(CFLAGS) $(CPPFLAGS) -c $< -o $@ clean: $(RM) *.o xxhsum xxhsum.exe ``` And the target clones can be implemented like this: ```c /* * A dummy file to make dispatching easier for the build system. */ #define XXH_STATIC_LINKING_ONLY /* access advanced declarations */ #define XXH_IMPLEMENTATION /* access definitions */ #define XXH3_DISPATCH /* Mark a dispatcher target */ #define XXH_VECTOR XXH_SSE2 /* Set target */ #define XXH_INLINE_ALL /* Disable other symbols, only emit a table */ #include "../xxhash.h" ```
easyaspi314 commented 2020-05-11 14:40:02 -05:00 (Migrated from github.com)

Should we also break off at XXH3_digest_long? My reasoning is that most of the time, we are only calling XXH3_accumulate_512 a few times, and it isn't the worst thing in the world if we take the scalar path.

Should we also break off at `XXH3_digest_long`? My reasoning is that *most* of the time, we are only calling `XXH3_accumulate_512` a few times, and it isn't the worst thing in the world if we take the scalar path.
easyaspi314 commented 2020-05-11 18:16:20 -05:00 (Migrated from github.com)

I was thinking of some alternative approaches.

First of all, we could use a function like this:

/* not a global */
static const XXH3_dispatch_table_t *XXH3_dispatch_table = NULL;
const XXH3_dispatch_table_t *
XXH3_get_dispatch_table(void)
{
    if (XXH_unlikely(XXH3_dispatch_table == NULL)) {
        const XXH3_dispatch_table_t *dispatch_table = XXH3_featureTest();
        XXH3_dispatch_table = dispatch_table;
        return dispatch_table;
    }
    return XXH3_dispatch_table;
}

This has a few advantages:

  • Better encapsulation
    • Dispatch table is impossible to modify without raw memory addresses (assuming I add the missing const to the target table pointers)
    • Can't do XXH3_dispatch_table = evil_table
  • Reduces code size and complexity
    • No need for the kDispatchDispatcher table and copy-pasted functions

However, it has to deal with the overhead of a function call and a branch.

We could also cache it like this:

XXH_PUBLIC_API XXH64_hash_t XXH3_64bits(const void* input, size_t len)
{
    if (len <= 16)
        return XXH3_len_0to16_64b((const xxh_u8*)input, len, kSecret, 0);
    if (len <= 128)
        return XXH3_len_17to128_64b((const xxh_u8*)input, len, kSecret, sizeof(kSecret), 0);
    if (len <= XXH3_MIDSIZE_MAX)
         return XXH3_len_129to240_64b((const xxh_u8*)input, len, kSecret, sizeof(kSecret), 0);
#ifdef XXH3_DISPATCH
    { static XXH64_hash_t (*dispatcher)(const xxh_u8*, size_t) = NULL;
       if (XXH_unlikely(dispatcher == NULL)) {
            dispatcher = &XXH3_get_dispatch_table()->hashLong_64b_defaultSecret;
        }
        return dispatcher((const xxh_u8*)input, len);
    }
#else
    return XXH3_hashLong_64b_defaultSecret((const xxh_u8*)input, len);
#endif
}

This avoids a function call on subsequent calls, but will require local static variables for each inlining with XXH_INLINE_ALL, which from what I am seeing, potentially blocks inlining. It also makes the macro wrapper impossible without some fancy GCC extensions.

I was thinking of some alternative approaches. First of all, we could use a function like this: ```c /* not a global */ static const XXH3_dispatch_table_t *XXH3_dispatch_table = NULL; const XXH3_dispatch_table_t * XXH3_get_dispatch_table(void) { if (XXH_unlikely(XXH3_dispatch_table == NULL)) { const XXH3_dispatch_table_t *dispatch_table = XXH3_featureTest(); XXH3_dispatch_table = dispatch_table; return dispatch_table; } return XXH3_dispatch_table; } ``` This has a few advantages: - Better encapsulation - Dispatch table is impossible to modify without raw memory addresses (assuming I add the missing const to the target table pointers) - Can't do `XXH3_dispatch_table = evil_table` - Reduces code size and complexity - No need for the `kDispatchDispatcher` table and copy-pasted functions However, it has to deal with the overhead of a function call and a branch. We could also cache it like this: ```c XXH_PUBLIC_API XXH64_hash_t XXH3_64bits(const void* input, size_t len) { if (len <= 16) return XXH3_len_0to16_64b((const xxh_u8*)input, len, kSecret, 0); if (len <= 128) return XXH3_len_17to128_64b((const xxh_u8*)input, len, kSecret, sizeof(kSecret), 0); if (len <= XXH3_MIDSIZE_MAX) return XXH3_len_129to240_64b((const xxh_u8*)input, len, kSecret, sizeof(kSecret), 0); #ifdef XXH3_DISPATCH { static XXH64_hash_t (*dispatcher)(const xxh_u8*, size_t) = NULL; if (XXH_unlikely(dispatcher == NULL)) { dispatcher = &XXH3_get_dispatch_table()->hashLong_64b_defaultSecret; } return dispatcher((const xxh_u8*)input, len); } #else return XXH3_hashLong_64b_defaultSecret((const xxh_u8*)input, len); #endif } ``` This avoids a function call on subsequent calls, but will require local static variables for each inlining with `XXH_INLINE_ALL`, which from what I am seeing, potentially blocks inlining. It also makes the macro wrapper impossible without some fancy GCC extensions.
Cyan4973 commented 2020-05-11 18:47:17 -05:00 (Migrated from github.com)

I like this last version.
Caching the result in a local static variable is something I was going to recommend.
I don't see any problem regarding inlining on my side (though of course I could always be wrong).
The only issue I could guess is a risk of double-initialization in multi-threaded environments, but even that is not a big deal, since initialization's outcome is presumed the same.

I like this last version. Caching the result in a local static variable is something I was going to recommend. I don't see any problem regarding inlining on my side (though of course I could always be wrong). The only issue I could guess is a risk of double-initialization in multi-threaded environments, but even that is not a big deal, since initialization's outcome is presumed the same.
👍 1
gzm55 commented 2020-05-11 19:16:04 -05:00 (Migrated from github.com)

Self caching still works for XXH3_get_dispatch_table.

static const XXH3_dispatch_table_t * (*XXH3_get_dispatch_table) ; // declaring

const XXH3_dispatch_table_t *
XXH3_get_dispatch_table_no_check(void)
{ return XXH3_dispatch_table; }

const XXH3_dispatch_table_t *
XXH3_get_dispatch_table_init(void)
{
    XXH3_get_dispatch_table = &XXH3_get_dispatch_table_no_check;
    const XXH3_dispatch_table_t *dispatch_table = XXH3_featureTest();
    XXH3_dispatch_table = dispatch_table;
    return dispatch_table;
}

static const XXH3_dispatch_table_t *
(*XXH3_get_dispatch_table) = &XXH3_get_dispatch_table_init;

// in XXH3_64bits
#ifdef XXH3_DISPATCH
    { XXH64_hash_t (*dispatcher)(const xxh_u8*, size_t) = NULL;
        dispatcher = &XXH3_get_dispatch_table()->hashLong_64b_defaultSecret;
        return dispatcher((const xxh_u8*)input, len);
    }
#else

In addition of idempotency, also need to consider whether storing a function pointer is atomic.

Self caching still works for `XXH3_get_dispatch_table`. ```c static const XXH3_dispatch_table_t * (*XXH3_get_dispatch_table) ; // declaring const XXH3_dispatch_table_t * XXH3_get_dispatch_table_no_check(void) { return XXH3_dispatch_table; } const XXH3_dispatch_table_t * XXH3_get_dispatch_table_init(void) { XXH3_get_dispatch_table = &XXH3_get_dispatch_table_no_check; const XXH3_dispatch_table_t *dispatch_table = XXH3_featureTest(); XXH3_dispatch_table = dispatch_table; return dispatch_table; } static const XXH3_dispatch_table_t * (*XXH3_get_dispatch_table) = &XXH3_get_dispatch_table_init; // in XXH3_64bits #ifdef XXH3_DISPATCH { XXH64_hash_t (*dispatcher)(const xxh_u8*, size_t) = NULL; dispatcher = &XXH3_get_dispatch_table()->hashLong_64b_defaultSecret; return dispatcher((const xxh_u8*)input, len); } #else ``` In addition of idempotency, also need to consider whether storing a function pointer is atomic.
easyaspi314 commented 2020-05-11 19:22:34 -05:00 (Migrated from github.com)

That will still call a function each time.

If we didn't give a damn about compatibility and wanted to just use GCC extensions, I wrote a macro which would work with xxh3.h as-is:

const XXH3_dispatch_table_t *XXH3_get_dispatch_table(void);

#define XXH3_DISPATCH_CALL(x) __extension__ ({ \
    static __typeof__(&XXH3_##x) dispatch = NULL; \
    if (XXH_unlikely(dispatch == NULL)) { \
        dispatch = XXH3_get_dispatch_table()->x; \
    } \
    dispatch; \
})
That will still call a function each time. If we didn't give a damn about compatibility and wanted to just use GCC extensions, I wrote a macro which would work with xxh3.h as-is: ```c const XXH3_dispatch_table_t *XXH3_get_dispatch_table(void); #define XXH3_DISPATCH_CALL(x) __extension__ ({ \ static __typeof__(&XXH3_##x) dispatch = NULL; \ if (XXH_unlikely(dispatch == NULL)) { \ dispatch = XXH3_get_dispatch_table()->x; \ } \ dispatch; \ }) ```
easyaspi314 commented 2020-05-11 20:33:12 -05:00 (Migrated from github.com)

In addition of idempotency, also need to consider whether storing a function pointer is atomic.

So let's look at possible data race issues:

  1. Something reads while the pointer is being written.
  2. Something writes while the pointer is being written.
  3. Something writes while the pointer is being read.

We can immediately eliminate condition 2 because the feature test will be deterministic on any sane system, so a torn write will still end up storing the same value.

On all known platforms, sizeof(void (*)()) == sizeof(void *)

Therefore, 1 and 3 can also be eliminated by the x86 and C standards:

  • x86 guarantees that 32-bit (and 64-bit on x86_64) loads and stores are atomic, if they are naturally aligned.
  • C guarantees that all variables will be naturally aligned by default.

This aligned store atomicity is also guaranteed by non-M ARMv6 and later (you can't combine A/R/M binaries, and NEON is only on A), aarch64, and if I am reading correctly, PowerPC and s390x.

Therefore, it is safe.

> In addition of idempotency, also need to consider whether storing a function pointer is atomic. So let's look at possible data race issues: 1. Something reads while the pointer is being written. 2. Something writes while the pointer is being written. 3. Something writes while the pointer is being read. We can immediately eliminate condition 2 because the feature test will be deterministic on any sane system, so a torn write will still end up storing the same value. On all known platforms, `sizeof(void (*)()) == sizeof(void *)` Therefore, 1 and 3 can also be eliminated by the x86 and C standards: - x86 guarantees that 32-bit (and 64-bit on x86_64) loads and stores are *atomic*, if they are naturally aligned. - C guarantees that all variables will be naturally aligned by default. This aligned store atomicity is also guaranteed by non-M ARMv6 and later (you can't combine A/R/M binaries, and NEON is only on A), aarch64, and if I am reading correctly, PowerPC and s390x. Therefore, it is safe.
👍 1
gzm55 commented 2020-05-11 21:12:08 -05:00 (Migrated from github.com)

how about passing the runtime feature into hashLong_64b_defaultSecret and select the instruction branches via switch .. case, this should only have one jumping cost:

// in XXH3_accumulate_512
switch(cpu_feature) {
case sse2: // sse codes
break;
default: // scalar codes
break
}

At entrance of hashLong, we can still use self caching way to initialize the cpu_feature with O(1) times. The caching values is a function pointer (as previous analyzed, this should be data-racing safe) of hashLong_64b_defaultSecret. So another runtime cost is an indirect call, this could not be avoided in any runtime dispatching methods.

// global
static ...
hashLong_64b_defaultSecret_check(..., hashLongPtr_t *hashLong_64b_defaultSecret_ptr, int *cpu_feature)
{
  *cpu_feature = XXH3_featureTest();
  *hashLong_64b_defaultSecret_ptr = &hashLong_64b_defaultSecret_no_check;
  hashLong_64b_defaultSecret_no_check(..., null, *cpu_feature);
}

static ...
hashLong_64b_defaultSecret_no_check(..., hashLongPtr_t *hashLong_64b_defaultSecret_ptr, int *cpu_feature)
{
  // if defined XXH3_DISPATCH inline switch case 
  // if not defined, expand to static dispatching way
  switch(cpu_feature) {
  case sse2: // sse codes
  break;
  default: // scalar codes
  break
  }
}

// in XXH3_64bits
{
  static int cpu_feature = 0;
  static ... (*hashLong_64b_defaultSecret_ptr)=&hashLong_64b_defaultSecret_check;
  hashLong_64b_defaultSecret_ptr(..., &hashLong_64b_defaultSecret_ptr, &cpu_feature);
}

When using GCC, we can merge hashLong_64b_defaultSecret_check and hashLong_64b_defaultSecret_no_check into one function, and initialize the cpu feature with label as values extension:

// global
static ...
hashLong_64b_defaultSecret(...)
{
static void * label = &&init
static int cpu_feature = -1;
goto *label;
init:
  cpu_feature = XXH3_featureTest();
  label = &&hash;
hash:
    switch(cpu_feature) { ... }
}
how about passing the runtime feature into hashLong_64b_defaultSecret and select the instruction branches via `switch .. case`, this should only have one jumping cost: ```c // in XXH3_accumulate_512 switch(cpu_feature) { case sse2: // sse codes break; default: // scalar codes break } ``` At entrance of `hashLong`, we can still use self caching way to initialize the `cpu_feature` with O(1) times. The caching values is a function pointer (as previous analyzed, this should be data-racing safe) of `hashLong_64b_defaultSecret`. So another runtime cost is an indirect call, this could not be avoided in any runtime dispatching methods. ```c // global static ... hashLong_64b_defaultSecret_check(..., hashLongPtr_t *hashLong_64b_defaultSecret_ptr, int *cpu_feature) { *cpu_feature = XXH3_featureTest(); *hashLong_64b_defaultSecret_ptr = &hashLong_64b_defaultSecret_no_check; hashLong_64b_defaultSecret_no_check(..., null, *cpu_feature); } static ... hashLong_64b_defaultSecret_no_check(..., hashLongPtr_t *hashLong_64b_defaultSecret_ptr, int *cpu_feature) { // if defined XXH3_DISPATCH inline switch case // if not defined, expand to static dispatching way switch(cpu_feature) { case sse2: // sse codes break; default: // scalar codes break } } // in XXH3_64bits { static int cpu_feature = 0; static ... (*hashLong_64b_defaultSecret_ptr)=&hashLong_64b_defaultSecret_check; hashLong_64b_defaultSecret_ptr(..., &hashLong_64b_defaultSecret_ptr, &cpu_feature); } ``` When using GCC, we can merge `hashLong_64b_defaultSecret_check` and `hashLong_64b_defaultSecret_no_check` into one function, and initialize the cpu feature with `label as values` extension: ```c // global static ... hashLong_64b_defaultSecret(...) { static void * label = &&init static int cpu_feature = -1; goto *label; init: cpu_feature = XXH3_featureTest(); label = &&hash; hash: switch(cpu_feature) { ... } } ```
easyaspi314 commented 2020-05-11 21:24:37 -05:00 (Migrated from github.com)

Well that also requires declarations of all the functions, and will also increase code size, require fallbacks for non-GCC compilers which we have to support. It also requires conditionals for when we don't have AVX2 or AVX512, and will just look really ugly.

The self caching function pointer seems to be pretty well optimized with Clang, even with PIC:

XXH3_128bits_withSeed:
        # ....
.LBB36_23:
        mov     rcx, qword ptr [rip + XXH3_128bits_withSeed.dispatch]
        test    rcx, rcx
        je      .LBB36_24
.LBB36_25:
        mov     rdi, r15
        mov     rsi, rbx
        mov     rdx, r14
        call    rcx
        pop     rbx
        pop     r14
        pop     r15
        ret
# Clang puts this at the very end of the function.
.LBB36_24:
        call    XXH3_get_dispatch_table@PLT
        mov     rcx, qword ptr [rax + 40] # ->hashLong_128b_withSeed
        mov     qword ptr [rip + XXH3_128bits_withSeed.dispatch], rcx
        jmp     .LBB36_25

Most of the time, we will be calling the same function over and over again, so the single caching seems ideal.

Well that also requires declarations of all the functions, and will also increase code size, require fallbacks for non-GCC compilers which we have to support. It also requires conditionals for when we don't have AVX2 or AVX512, and will just look really ugly. The self caching function pointer seems to be pretty well optimized with Clang, even with PIC: ```asm XXH3_128bits_withSeed: # .... .LBB36_23: mov rcx, qword ptr [rip + XXH3_128bits_withSeed.dispatch] test rcx, rcx je .LBB36_24 .LBB36_25: mov rdi, r15 mov rsi, rbx mov rdx, r14 call rcx pop rbx pop r14 pop r15 ret # Clang puts this at the very end of the function. .LBB36_24: call XXH3_get_dispatch_table@PLT mov rcx, qword ptr [rax + 40] # ->hashLong_128b_withSeed mov qword ptr [rip + XXH3_128bits_withSeed.dispatch], rcx jmp .LBB36_25 ``` Most of the time, we will be calling the same function over and over again, so the single caching seems ideal.
easyaspi314 commented 2020-05-11 21:56:11 -05:00 (Migrated from github.com)

Hmm.
Trying to figure out the balance between sanity and readability.

I don't want to write this over and over:

#ifdef XXH3_DISPATCH
    { static XXH128_hash_t (*dispatch_cache)(const xxh_u8 *, size_t, XXH64_hash_t) = NULL;
      if (XXH_unlikely(dispatch_cache == NULL)) {
          dispatch_cache = XXH3_get_dispatch_table()->hashLong_128b_withSeed;
      }
      return dispatch_cache((const xxh_u8 *)input, len, seed);
    }
#else
    return XXH3_hashLong_128bits_withSeed((const xxh_u8*)input, len, seed);
#endif

So my idea could be this:

#ifdef XXH3_DISPATCH
# define XXH3_DISPATCH_RETURN(rettype, name, paramlist, args) \
    do { \
        static rettype (*_dispatch_cache) paramlist = NULL; \
        if (XXH_unlikely(_dispatch_cache == NULL)) { \
            _dispatch_cache = XXH3_get_dispatch_table()->name; \
        } \
        return _dispatch_cache args; \
    } while (0)
#else
#  define XXH3_DISPATCH_RETURN(rettype, name, paramlist, args) \
    return XXH3_##name args
#endif


    XXH3_DISPATCH_RETURN(
        XXH128_hash_t, 
        hashLong_128b_withSeed, (const xxh_u8*, size_t, XXH64_hash_t),
        ((const xxh_u8*)input, len, seed)
    );

Is that readable/sane enough?

Life would be so much easier with typeof and statement expressions... I could write this in half the lines of code.

Hmm. Trying to figure out the balance between sanity and readability. I don't want to write this over and over: ```c #ifdef XXH3_DISPATCH { static XXH128_hash_t (*dispatch_cache)(const xxh_u8 *, size_t, XXH64_hash_t) = NULL; if (XXH_unlikely(dispatch_cache == NULL)) { dispatch_cache = XXH3_get_dispatch_table()->hashLong_128b_withSeed; } return dispatch_cache((const xxh_u8 *)input, len, seed); } #else return XXH3_hashLong_128bits_withSeed((const xxh_u8*)input, len, seed); #endif ``` So my idea could be this: ```c #ifdef XXH3_DISPATCH # define XXH3_DISPATCH_RETURN(rettype, name, paramlist, args) \ do { \ static rettype (*_dispatch_cache) paramlist = NULL; \ if (XXH_unlikely(_dispatch_cache == NULL)) { \ _dispatch_cache = XXH3_get_dispatch_table()->name; \ } \ return _dispatch_cache args; \ } while (0) #else # define XXH3_DISPATCH_RETURN(rettype, name, paramlist, args) \ return XXH3_##name args #endif XXH3_DISPATCH_RETURN( XXH128_hash_t, hashLong_128b_withSeed, (const xxh_u8*, size_t, XXH64_hash_t), ((const xxh_u8*)input, len, seed) ); ``` Is that readable/sane enough? Life would be so much easier with `typeof` and statement expressions... I could write this in half the lines of code.
gzm55 commented 2020-05-11 22:39:06 -05:00 (Migrated from github.com)

with typedef we could avoid pass paramlist every time

typedef XXH128_hash_t (*hashLong_XXH128_hash_t)(const xxh_u8*, size_t, XXH64_hash_t);
typedef XXH64_hash_t (*hashLong_XXH64_hash_t)(const xxh_u8*, size_t, XXH64_hash_t);
with `typedef` we could avoid pass `paramlist` every time ```c typedef XXH128_hash_t (*hashLong_XXH128_hash_t)(const xxh_u8*, size_t, XXH64_hash_t); typedef XXH64_hash_t (*hashLong_XXH64_hash_t)(const xxh_u8*, size_t, XXH64_hash_t);
👍 1
easyaspi314 commented 2020-05-11 22:47:34 -05:00 (Migrated from github.com)

I totally didn't think of that. That would make things pretty simple with some token pasting magic.

I totally didn't think of that. That would make things pretty simple with some token pasting magic.
gzm55 commented 2020-05-12 02:18:01 -05:00 (Migrated from github.com)

Try to simplify the XXH3_DISPATCH_RETURN macro:

#ifdef XXH3_DISPATCH
# define XXH3_DISPATCH_RETURN(name, ...) \
    do { \
        static ptrdiff_t _dispatch_cache = 1; \
        if (XXH_unlikely(_dispatch_cache == 1)) { \
            _dispatch_cache = XXH3_get_dispatch_table()->name - &XXH3_##name##_scalar; \
        } \
        return (&XXH3_##name##_scalar + _dispatch_cache)(__VA_ARGS__); \
    } while (0)
#else
#  define XXH3_DISPATCH_RETURN(name, ...) \
    return XXH3_##name(__VA_ARGS__)
#endif


    XXH3_DISPATCH_RETURN(hashLong_128b_withSeed, (const xxh_u8*)input, len, seed);
Try to simplify the `XXH3_DISPATCH_RETURN` macro: ```c #ifdef XXH3_DISPATCH # define XXH3_DISPATCH_RETURN(name, ...) \ do { \ static ptrdiff_t _dispatch_cache = 1; \ if (XXH_unlikely(_dispatch_cache == 1)) { \ _dispatch_cache = XXH3_get_dispatch_table()->name - &XXH3_##name##_scalar; \ } \ return (&XXH3_##name##_scalar + _dispatch_cache)(__VA_ARGS__); \ } while (0) #else # define XXH3_DISPATCH_RETURN(name, ...) \ return XXH3_##name(__VA_ARGS__) #endif XXH3_DISPATCH_RETURN(hashLong_128b_withSeed, (const xxh_u8*)input, len, seed); ```
easyaspi314 commented 2020-05-12 08:37:52 -05:00 (Migrated from github.com)

We can't use variadic macros, those require C99. Besides, that would need more global symbols, and you can't do pointer arithmetic on function pointers.

The typedef solution works.

We can't use variadic macros, those require C99. Besides, that would need more global symbols, and you can't do pointer arithmetic on function pointers. The typedef solution works.
easyaspi314 commented 2020-05-12 12:48:02 -05:00 (Migrated from github.com)

Hmm, QEMU, being quirky as it is, will actually return 0 cpuid leaves on -cpu base. (Yes, I am running QEMU on my phone 😛)

$ qemu-i386 -cpu base xxhsum -b
xxhsum 0.7.4 by Yann Collet, compiled as 32-bit i386 little endian with Clang 9.0.1
DEBUG: xxHash dispatch: Max CPUID leaves == 0!
DEBUG: xxHash dispatch: Setting dispatch table to XXH_SCALAR.
Sample of 100 KB...
XXH32                         :     102400 ->     6703 it/s (  654.6 MB/s)
XXH32 unaligned               :     102400 ->     6266 it/s (  611.9 MB/s)
XXH64                         :     102400 ->     3530 it/s (  344.7 MB/s)
XXH64 unaligned               :     102400 ->     3559 it/s (  347.6 MB/s)
XXH3_64b                      :     102400 ->     5591 it/s (  546.0 MB/s)
XXH3_64b unaligned            :     102400 ->     5545 it/s (  541.5 MB/s)
XXH3_64b w/seed               :     102400 ->     5674 it/s (  554.1 MB/s)
XXH3_64b w/seed unaligned     :     102400 ->     5760 it/s (  562.5 MB/s)
XXH3_64b w/secret             :     102400 ->     5189 it/s (  506.7 MB/s)
XXH3_64b w/secret unaligned   :     102400 ->     5136 it/s (  501.6 MB/s)
XXH128                        :     102400 ->     4653 it/s (  454.4 MB/s)
XXH128 unaligned              :     102400 ->     4622 it/s (  451.3 MB/s)
XXH128 w/seed                 :     102400 ->     4726 it/s (  461.5 MB/s)
XXH128 w/seed unaligned       :     102400 ->     4732 it/s (  462.1 MB/s)
XXH128 w/secret               :     102400 ->     4420 it/s (  431.6 MB/s)
XXH128 w/secret unaligned     :     102400 ->     4448 it/s (  434.3 MB/s)

Will do some cleanup, add some more debugging code, and push soon.

Hmm, QEMU, being quirky as it is, will actually return 0 cpuid leaves on `-cpu base`. (Yes, I am running QEMU on my phone 😛) ``` $ qemu-i386 -cpu base xxhsum -b xxhsum 0.7.4 by Yann Collet, compiled as 32-bit i386 little endian with Clang 9.0.1 DEBUG: xxHash dispatch: Max CPUID leaves == 0! DEBUG: xxHash dispatch: Setting dispatch table to XXH_SCALAR. Sample of 100 KB... XXH32 : 102400 -> 6703 it/s ( 654.6 MB/s) XXH32 unaligned : 102400 -> 6266 it/s ( 611.9 MB/s) XXH64 : 102400 -> 3530 it/s ( 344.7 MB/s) XXH64 unaligned : 102400 -> 3559 it/s ( 347.6 MB/s) XXH3_64b : 102400 -> 5591 it/s ( 546.0 MB/s) XXH3_64b unaligned : 102400 -> 5545 it/s ( 541.5 MB/s) XXH3_64b w/seed : 102400 -> 5674 it/s ( 554.1 MB/s) XXH3_64b w/seed unaligned : 102400 -> 5760 it/s ( 562.5 MB/s) XXH3_64b w/secret : 102400 -> 5189 it/s ( 506.7 MB/s) XXH3_64b w/secret unaligned : 102400 -> 5136 it/s ( 501.6 MB/s) XXH128 : 102400 -> 4653 it/s ( 454.4 MB/s) XXH128 unaligned : 102400 -> 4622 it/s ( 451.3 MB/s) XXH128 w/seed : 102400 -> 4726 it/s ( 461.5 MB/s) XXH128 w/seed unaligned : 102400 -> 4732 it/s ( 462.1 MB/s) XXH128 w/secret : 102400 -> 4420 it/s ( 431.6 MB/s) XXH128 w/secret unaligned : 102400 -> 4448 it/s ( 434.3 MB/s) ``` Will do some cleanup, add some more debugging code, and push soon.
easyaspi314 commented 2020-05-17 11:14:24 -05:00 (Migrated from github.com)

Is there anyone who is good at CMake who can give the dispatcher code a shot? Because all I see is something hopelessly messy or verbose.

Is there anyone who is good at CMake who can give the dispatcher code a shot? Because all I see is something hopelessly messy or verbose.
Cyan4973 commented 2020-05-29 17:09:43 -05:00 (Migrated from github.com)

There are a lots of good things in this PR.
I do believe that the runtime logic is good.

My only concern is about build's complexity, which will make integrating this solution into a 3rd party application pretty difficult.

I believe it's possible to offer the same dispatching capability without the build complexity, but a solution would rely on a capability which doesn't exist yet : combine XXH_INLINE_ALL with XXH_NAMESPACE.

We could move forward on this proposal in 2 different ways. Either :

  • we merge the current PR (after fixing latest conflicts), and I'll add the inline+namespace capability
  • I develop the inline+namespace capability first, and you get to use it in your PR later.

I'm happy with any of these. The most important at this stage is to move forward.

edit : I may have underestimated the difficulty with static functions, however there are other possible solutions with same outcome : generate all possible vector variants, so that they can then be selected at runtime.

There are a lots of good things in this PR. I do believe that the runtime logic is good. My only concern is about build's complexity, which will make integrating this solution into a 3rd party application pretty difficult. I believe it's possible to offer the same dispatching capability without the build complexity, but a solution would rely on a capability which doesn't exist yet : combine `XXH_INLINE_ALL` with `XXH_NAMESPACE`. We could move forward on this proposal in 2 different ways. Either : - we merge the current PR (after fixing latest conflicts), and I'll add the inline+namespace capability - I develop the inline+namespace capability first, and you get to use it in your PR later. I'm happy with any of these. The most important at this stage is to move forward. _edit_ : I may have underestimated the difficulty with `static` functions, however there are other possible solutions with same outcome : generate all possible vector variants, so that they can then be selected at runtime.
Cyan4973 commented 2020-05-29 19:17:23 -05:00 (Migrated from github.com)

In branch multiVersions, there is an unfinished PoC which hopefully exposes clearly enough the proposed design.

In this branch, and given proper compilation flags (such as -mavx512f), the compiler will generate all possible variants (in this case, scalar, sse2, avx2 and avx512f), and make them accessible with relevant function name suffix.
Then, left alone, it picks only one of them, and rely on the Dead Code Elimination to get rid of the unused ones.

This is achieved without any modification to the build script. Eventually, this should allow a more evolved runtime selection to pick any of them, keeping them all present in the final binary.

edit : in branch testDispatch2, this PR has been merged with multiVersions, fixing a few conflicts in the process.

In branch [`multiVersions`](https://github.com/Cyan4973/xxHash/tree/multiVersions), there is an unfinished PoC which hopefully exposes clearly enough the proposed design. In this branch, and given proper compilation flags (such as `-mavx512f`), the compiler will generate _all_ possible variants (in this case, `scalar`, `sse2`, `avx2` and `avx512f`), and make them accessible with relevant function name suffix. Then, left alone, it picks only one of them, and rely on the Dead Code Elimination to get rid of the unused ones. This is achieved without any modification to the build script. Eventually, this should allow a more evolved runtime selection to pick _any_ of them, keeping them all present in the final binary. _edit_ : in branch `testDispatch2`, this PR has been merged with `multiVersions`, fixing a few conflicts in the process.
easyaspi314 commented 2020-05-29 21:20:43 -05:00 (Migrated from github.com)

Well we still can't rely on that, sadly.

Intrinsics are more complicated than they look, and the compiler can replace intrinsics with better instructions if they are available.

#include <emmintrin.h>

__m128i blend(__m128i x, __m128i y)
{
    const __m128i x_mask = _mm_setr_epi32(0x00000000, 0xffffffff, 0x00000000, 0xffffffff);
    const __m128i y_mask = _mm_setr_epi32(0xffffffff, 0x00000000, 0xffffffff, 0x00000000);
    __m128i x_masked = _mm_and_si128(x, x_mask);
    __m128i y_masked = _mm_and_si128(y, y_mask);
    return _mm_or_si128(x_masked, y_masked);
}

The code you might expect would be this:

.Lx_mask:
        .dword  0x00000000
        .dword  0xFFFFFFFF
        .dword  0x00000000
        .dword  0xFFFFFFFF
.Ly_mask:
        .dword  0xFFFFFFFF
        .dword  0x00000000
        .dword  0xFFFFFFFF
        .dword  0x00000000
blend:
        pand    xmm0, xmmword ptr [.Lx_mask]
        pand    xmm1, xmmword ptr [.Ly_mask]
        por     xmm0, xmm1
        ret

But Clang emits this on -msse2:

blend:
        pshufd  xmm2, xmm0, _MM_SHUFFLE(3,2,3,1)
        pshufd  xmm0, xmm1, _MM_SHUFFLE(3,2,2,0)
        punpckldq xmm0, xmm2
        ret

This on -msse4.1:

blend:
        # requires sse4.1
        blendps  xmm0, xmm1, 5
        ret

And this on -mavx:

blend:
        # requires avx (modern vex encoding)
        vblendps xmm0, xmm1, xmm0, 10
        ret

Additionally, we need to count autovectorization which can also emit instructions that we don't want.

We could do __attribute__((__target__)) on GCC, but we would have to know whether the GCC/Clang version supports said target, which you can't easily check. It also isn't portable to MSVC and Clang doesn't do a good job at respecting it.

Well we still can't rely on that, sadly. Intrinsics are more complicated than they look, and the compiler can replace intrinsics with better instructions if they are available. ```c #include <emmintrin.h> __m128i blend(__m128i x, __m128i y) { const __m128i x_mask = _mm_setr_epi32(0x00000000, 0xffffffff, 0x00000000, 0xffffffff); const __m128i y_mask = _mm_setr_epi32(0xffffffff, 0x00000000, 0xffffffff, 0x00000000); __m128i x_masked = _mm_and_si128(x, x_mask); __m128i y_masked = _mm_and_si128(y, y_mask); return _mm_or_si128(x_masked, y_masked); } ``` The code you might expect would be this: ```asm .Lx_mask: .dword 0x00000000 .dword 0xFFFFFFFF .dword 0x00000000 .dword 0xFFFFFFFF .Ly_mask: .dword 0xFFFFFFFF .dword 0x00000000 .dword 0xFFFFFFFF .dword 0x00000000 blend: pand xmm0, xmmword ptr [.Lx_mask] pand xmm1, xmmword ptr [.Ly_mask] por xmm0, xmm1 ret ``` But Clang emits this on -msse2: ```asm blend: pshufd xmm2, xmm0, _MM_SHUFFLE(3,2,3,1) pshufd xmm0, xmm1, _MM_SHUFFLE(3,2,2,0) punpckldq xmm0, xmm2 ret ``` This on -msse4.1: ```asm blend: # requires sse4.1 blendps xmm0, xmm1, 5 ret ``` And this on -mavx: ```asm blend: # requires avx (modern vex encoding) vblendps xmm0, xmm1, xmm0, 10 ret ``` Additionally, we need to count autovectorization which can also emit instructions that we don't want. We *could* do `__attribute__((__target__))` on GCC, but we would have to know whether the GCC/Clang version supports said target, which you can't easily check. It also isn't portable to MSVC *and* Clang doesn't do a good job at respecting it.
👍 2
easyaspi314 commented 2020-05-29 21:33:19 -05:00 (Migrated from github.com)

As for the build system, 90% of it is feature tests.

It essentially boils down to this:

CFLAGS := -O2 -g -march=x86-64 -DXXH_DISPATCH -DXXH_DISPATCH_AVX2 -DXXH_DISPATCH_AVX512

XXHASH_SRCS := xxhash-dispatch.c xxhash-scalar.c xxhash-sse2.c xxhash-avx2.c xxhash-avx512.c
XXHASH_OBJS := $(XXHASH_SRCS:.c=.o)

$(XXHASH_OBJS): %.o: %.c xxhash.h xxh3.h
        $(CC) -c $(CFLAGS) $(TARGETFLAGS) $< -o $@

xxhash-sse2.o: TARGETFLAGS = -msse2
xxhash-avx2.o: TARGETFLAGS = -mavx2
xxhash-avx512.o: TARGETFLAGS = -mavx512f
As for the build system, 90% of it is feature tests. It essentially boils down to this: ```make CFLAGS := -O2 -g -march=x86-64 -DXXH_DISPATCH -DXXH_DISPATCH_AVX2 -DXXH_DISPATCH_AVX512 XXHASH_SRCS := xxhash-dispatch.c xxhash-scalar.c xxhash-sse2.c xxhash-avx2.c xxhash-avx512.c XXHASH_OBJS := $(XXHASH_SRCS:.c=.o) $(XXHASH_OBJS): %.o: %.c xxhash.h xxh3.h $(CC) -c $(CFLAGS) $(TARGETFLAGS) $< -o $@ xxhash-sse2.o: TARGETFLAGS = -msse2 xxhash-avx2.o: TARGETFLAGS = -mavx2 xxhash-avx512.o: TARGETFLAGS = -mavx512f ```
👍 1
Cyan4973 commented 2020-05-29 22:51:32 -05:00 (Migrated from github.com)

__attribute__((__target__)) is the next logical step.
It can be combined with *_avx2() variants to become *_forceavx2() in the dispatcher.

One of the benefits of having a runtime instruction dispatcher in its own unit is that it doesn't have to match the very high level of portability that the library itself must provide.
Which means, I'm fine with a first solution limited to gcc / clang.

Clang doesn't do a good job at respecting it.

That would be more problematic.
FYI, we do use __attribute__((__target__)) on zstd, and haven't met (yet) an issue with clang. So it felt safe. That being said, zstd doesn't have an avx512f code path either.

`__attribute__((__target__))` is the next logical step. It can be combined with `*_avx2()` variants to become `*_forceavx2()` in the dispatcher. One of the benefits of having a runtime instruction dispatcher in its own unit is that it doesn't have to match the very high level of portability that the library itself must provide. Which means, I'm fine with a first solution limited to `gcc` / `clang`. > Clang doesn't do a good job at respecting it. That would be more problematic. FYI, we do use `__attribute__((__target__))` on `zstd`, and haven't met (yet) an issue with `clang`. So it felt safe. That being said, `zstd` doesn't have an `avx512f` code path either.
Cyan4973 commented 2020-06-03 16:36:41 -05:00 (Migrated from github.com)

The debate around build-driven vs source-driven dispatcher generation is partially related to which use case is going to be supported.

If the objective is to make it part of xxhsum, and limit the "official" support to this one use case,
then build-driven is acceptable. We'll have to update the provided build systems, starting with Makefile, and likely its cmake equivalent, but hey, that's part of the project, so that's directly manageable. Any further usage is "unsupported".

If the objective is to support integration of the dispatcher into 3rd party applications, I believe the build-driven approach will significantly dent its reach. That's where source-driven becomes superior, even if its support is limited to some (common) compilers.
Eventually, if there is an ability to offer both strategies, source-driven for supported compilers, build-driven for all other ones, then it would join the best of both world.

To be fair, I'm comfortable with the idea that the dispatcher is for xxhsum only, at least for the time being, and reconsider its scope later on. But I remember some comments stating that they wish to integrate this capability into their own applications, hence I'm not completely sure if xxhsum-only is a good enough scope.

The debate around build-driven vs source-driven dispatcher generation is partially related to which use case is going to be supported. If the objective is to make it part of `xxhsum`, and limit the "official" support to this one use case, then build-driven is acceptable. We'll have to update the provided build systems, starting with `Makefile`, and likely its `cmake` equivalent, but hey, that's part of the project, so that's directly manageable. Any further usage is "unsupported". If the objective is to support integration of the dispatcher into 3rd party applications, I believe the build-driven approach will significantly dent its reach. That's where source-driven becomes superior, even if its support is limited to _some_ (common) compilers. Eventually, if there is an ability to offer both strategies, source-driven for supported compilers, build-driven for all other ones, then it would join the best of both world. To be fair, I'm comfortable with the idea that the dispatcher is for `xxhsum` only, at least for the time being, and reconsider its scope later on. But I remember some comments stating that they wish to integrate this capability into their own applications, hence I'm not completely sure if `xxhsum`-only is a good enough scope.
Cyan4973 (Migrated from github.com) reviewed 2020-06-03 16:44:20 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
Cyan4973 (Migrated from github.com) commented 2020-06-03 16:44:20 -05:00

One annoyance in this PR is that there is a significant amount of code for the dispatcher inside xxh3.h.
Not that I'm expecting xxh3.h to remain unmodified, it's clear that it is necessary to add a few hooks,
but I was wondering if changes within xxh3.h could be reduced, and rather transferred into xxhash-dispatch.c.
For example, they could be defined within xxhash-dispatch.c before including xxhash.h

One annoyance in this PR is that there is a significant amount of code for the dispatcher inside `xxh3.h`. Not that I'm expecting `xxh3.h` to remain unmodified, it's clear that it is necessary to add a few hooks, but I was wondering if changes within `xxh3.h` could be reduced, and rather transferred into `xxhash-dispatch.c`. For example, they could be defined within `xxhash-dispatch.c` _before_ including `xxhash.h`
easyaspi314 (Migrated from github.com) reviewed 2020-06-03 17:06:28 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
easyaspi314 (Migrated from github.com) commented 2020-06-03 17:06:28 -05:00

Well we could have an ifndef/define guard and put everything dispatch in another file, but that option causes issues with most XXH_INLINE_ALL uses.

If you are fine with that, I'm fine with that.

Alternatively, we could have xxhash.h include a different header if XXH_DISPATCH is defined, but that makes things complicated with build paths and can be ugly when it is installed as a system header.

Well we could have an `ifndef/define` guard and put everything dispatch in another file, but that option causes issues with most `XXH_INLINE_ALL` uses. If you are fine with that, I'm fine with that. Alternatively, we could have `xxhash.h` include a different header if `XXH_DISPATCH` is defined, but that makes things complicated with build paths and can be ugly when it is installed as a system header.
Cyan4973 (Migrated from github.com) reviewed 2020-06-03 17:10:48 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
Cyan4973 (Migrated from github.com) commented 2020-06-03 17:10:48 -05:00

that option causes issues with most XXH_INLINE_ALL uses.

Well, the objective being to reduce potential impact on normal behavior of xxhash.h,
this outcome wouldn't be fine.

That being said, I'm struggling to understand a scenario where inlining would be impacted.

> that option causes issues with most `XXH_INLINE_ALL` uses. Well, the objective being to reduce potential impact on normal behavior of `xxhash.h`, this outcome wouldn't be fine. That being said, I'm struggling to understand a scenario where inlining would be impacted.
easyaspi314 (Migrated from github.com) reviewed 2020-06-04 12:03:02 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
easyaspi314 (Migrated from github.com) commented 2020-06-04 12:03:01 -05:00

Unless you include the special header beforehand, the preinclude method would only use the normal code path with XXH_INLINE_ALL.

The include from xxhash.h adds another header to /usr/include, which is unwanted.

Unless you include the special header beforehand, the preinclude method would only use the normal code path with XXH_INLINE_ALL. The include from xxhash.h adds *another* header to `/usr/include`, which is unwanted.
Cyan4973 (Migrated from github.com) reviewed 2020-06-04 19:58:26 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
Cyan4973 (Migrated from github.com) commented 2020-06-04 19:58:26 -05:00

Unless you include the special header beforehand,

Well yes that's the idea

> Unless you include the special header beforehand, Well yes that's the idea
easyaspi314 (Migrated from github.com) reviewed 2020-06-04 20:22:33 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
easyaspi314 (Migrated from github.com) commented 2020-06-04 20:22:33 -05:00

I mean, we could nop the header on other arches.

So basically it could be like this

// xxhash-dispatch.h
#if defined(__i386__) || defined(__x86_64__) || ...

#define XXH_DISPATCH_CALL(...) ...
#define ...
...
#endif
#include "xxhash.h"
I mean, we could nop the header on other arches. So basically it could be like this ```c // xxhash-dispatch.h #if defined(__i386__) || defined(__x86_64__) || ... #define XXH_DISPATCH_CALL(...) ... #define ... ... #endif #include "xxhash.h" ```
Cyan4973 (Migrated from github.com) reviewed 2020-06-04 20:29:57 -05:00
xxh3.h
@ -156,2 +156,4 @@
#endif
/* ==========================================
* Dispatcher macros
Cyan4973 (Migrated from github.com) commented 2020-06-04 20:29:57 -05:00

Yes, exactly

Yes, exactly
Cyan4973 commented 2020-06-09 01:40:03 -05:00 (Migrated from github.com)

In branch x86dispatch, there is a proof of concept dispatch code, reusing verbatim the detection routine developed by @easyaspi314 in this PR, and coupling it with a dispatch strategy entirely source driven, based on __attribute__((__target__)), hence limited to a couple of supporting compilers.

The interesting points in this strategy :

  • simplified build : just include xxh_x86dispatch.h in the user program, it will swap the supported functions by the dispatch variants
  • no dispatch logic within libxxhash proper

The second point is pretty critical, as I'm quite afraid of making the main library more difficult to maintain for the sake of an optional capability.
It doesn't mean that the library is not modified. Clearly it is. But the dispatch logic is fully centralized within xxh_x86dispatch.c.

The main modifications within xxhash.h update some internal functions to accept function pointers as arguments. It makes it possible to swap the specialized implementation of accumulate512 and scrambleAcc with selective scalar / sse2 / avx2 vector implementations from a single main function body. No preprocessor trick is required : inlining function pointers works pretty well !

As a side effect, the dynamic dispatch runs the avx2 variant faster than the "regular" library compiled with -mavx2 : on my laptop, performance climbs from 50 GB/s to 60 GB/s, quite remarkable, and definitely way above noise level.

The PoC is not complete : at this stage, it's only able to replace 2 functions (XXH3_64bits() and XXH3_64bits_withSeed()). But it's already illustrative of selected design. Supporting more functions will be achieved by applying the same recipe almost mechanically.

Also, when compiled with -mavx512f, something in the detection routine makes the executable fail at runtime :
[1] 98930 illegal hardware instruction
This feels like an avx512 instruction is used on a cpu without avx512 support, which obviously doesn't end well. But it seems to happen inside XXH_featureTest(). Something to look at ...

In branch [x86dispatch](https://github.com/Cyan4973/xxHash/tree/x86dispatch), there is a proof of concept dispatch code, reusing verbatim the detection routine developed by @easyaspi314 in this PR, and coupling it with a dispatch strategy entirely source driven, based on `__attribute__((__target__))`, hence limited to a couple of supporting compilers. The interesting points in this strategy : - simplified build : just include `xxh_x86dispatch.h` in the user program, it will swap the supported functions by the dispatch variants - no dispatch logic within `libxxhash` proper The second point is pretty critical, as I'm quite afraid of making the main library more difficult to maintain for the sake of an optional capability. It doesn't mean that the library is not modified. Clearly it is. But the dispatch logic is fully centralized within `xxh_x86dispatch.c`. The main modifications within `xxhash.h` update some internal functions to accept function pointers as arguments. It makes it possible to swap the specialized implementation of `accumulate512` and `scrambleAcc` with selective `scalar` / `sse2` / `avx2` vector implementations from a single main function body. No preprocessor trick is required : inlining function pointers works pretty well ! As a side effect, the dynamic dispatch runs the `avx2` variant faster than the "regular" library compiled with `-mavx2` : on my laptop, performance climbs from 50 GB/s to 60 GB/s, quite remarkable, and definitely way above noise level. The PoC is not complete : at this stage, it's only able to replace 2 functions (`XXH3_64bits()` and `XXH3_64bits_withSeed()`). But it's already illustrative of selected design. Supporting more functions will be achieved by applying the same recipe almost mechanically. Also, when compiled with `-mavx512f`, something in the detection routine makes the executable fail at runtime : `[1] 98930 illegal hardware instruction` This feels like an `avx512` instruction is used on a cpu without `avx512` support, which obviously doesn't end well. But it seems to happen inside `XXH_featureTest()`. Something to look at ...
gzm55 commented 2020-06-09 02:43:12 -05:00 (Migrated from github.com)

when compiled with -mavx512f, something in the detection routine makes the executable fail at runtime :
[1] 98930 illegal hardware instruction
This feels like an avx512 instruction is used on a cpu without avx512 support, which obviously doesn't end well. But it seems to happen inside XXH_featureTest(). Something to look at ...

maybe we should add __attribute__((__target__ ("sse2"))) for all non-dispatch functions when compiling with x86 runtime dispatching feature.

> when compiled with `-mavx512f`, something in the detection routine makes the executable fail at runtime : > `[1] 98930 illegal hardware instruction` > This feels like an `avx512` instruction is used on a cpu without `avx512` support, which obviously doesn't end well. But it seems to happen inside `XXH_featureTest()`. Something to look at ... maybe we should add `__attribute__((__target__ ("sse2")))` for all non-dispatch functions when compiling with x86 runtime dispatching feature.
easyaspi314 commented 2020-06-09 11:03:43 -05:00 (Migrated from github.com)

Also, when compiled with -mavx512f, something in the detection routine makes the executable fail at runtime :
[1] 98930 illegal hardware instruction
This feels like an avx512 instruction is used on a cpu without avx512 support, which obviously doesn't end well. But it seems to happen inside XXH_featureTest(). Something to look at ...

You can only turn on features with __attribute__((target)), if you compile with -mavx512f, you are giving the compiler the green light to use AVX512 where it wants.

That is the main issue I have with __attribute__((target)): It can sometimes be unpredictable when not used correctly.

You would have to compile for a generic target and have the __attribute__((target)) turn on features.

> Also, when compiled with `-mavx512f`, something in the detection routine makes the executable fail at runtime : `[1] 98930 illegal hardware instruction` This feels like an `avx512` instruction is used on a cpu without `avx512` support, which obviously doesn't end well. But it seems to happen inside `XXH_featureTest()`. Something to look at ... You can only turn **on** features with `__attribute__((target))`, if you compile with `-mavx512f`, you are giving the compiler the green light to use AVX512 where it wants. That is the main issue I have with `__attribute__((target))`: It can sometimes be unpredictable when not used correctly. You would have to compile for a generic target and have the `__attribute__((target))` turn *on* features.
👍 1
Cyan4973 commented 2020-06-09 11:22:45 -05:00 (Migrated from github.com)

After further tests, and increasing the nb of traces,
I do no longer believe the issue to be located in XXH_featureTest().

Rather, it comes later, after the avx2 variant get selected.

You can only turn on features with __attribute__((target))

That would be an explanation.
I'm trying to shield the variant from the impact of auto-vectorization, by specifying (for avx2 variant) :
__attribute__((__target__ ("avx2"))) __attribute__((__target__ ("no-avx512f"))).

If the no-avx512f is not effective, that would explain the issue.
A pity that the compiler doesn't produce any warning, it seems to accept the __attribute__((__target__ ("no-avx512f"))) just fine, but if the directive is not effective, it's misleading.

After further tests, and increasing the nb of traces, I do no longer believe the issue to be located in `XXH_featureTest()`. Rather, it comes later, after the `avx2` variant get selected. > You can only turn __on__ features with `__attribute__((target))` That would be an explanation. I'm trying to shield the variant from the impact of auto-vectorization, by specifying (for `avx2` variant) : `__attribute__((__target__ ("avx2"))) __attribute__((__target__ ("no-avx512f")))`. If the `no-avx512f` is not effective, that would explain the issue. A pity that the compiler doesn't produce any warning, it seems to accept the `__attribute__((__target__ ("no-avx512f")))` just fine, but if the directive is not effective, it's misleading.
easyaspi314 commented 2020-06-09 12:50:21 -05:00 (Migrated from github.com)

Well it "accepts" it and sorta obeys it, but the issue is that -mavx512f turns avx512f on for the entire translation unit meaning we would have to turn it off for every other function. It also doesn't play along well with intrinsics from what experienced.

It is 100x safer to use target to turn on features.

Well it "accepts" it and *sorta* obeys it, but the issue is that `-mavx512f` turns `avx512f` on for the entire translation unit meaning we would have to turn it *off* for every other function. It also doesn't play along well with intrinsics from what experienced. It is 100x safer to use `target` to turn on features.
👍 1
Cyan4973 commented 2020-06-09 12:59:04 -05:00 (Migrated from github.com)

Good point.
Let's reverse the logic then.
I'll update the PoC later today to start from a minimal capability, and then selectively enable additional instruction sets.

Good point. Let's reverse the logic then. I'll update the PoC later today to start from a minimal capability, and then selectively enable additional instruction sets.
👍 1
Cyan4973 commented 2020-06-09 16:13:49 -05:00 (Migrated from github.com)

The x86dispatch PoC has been updated,
employing the new strategy of starting from a baseline (typically x64+sse2) and adding vector instructions selectively.

Everything seems to work fine. No more crash. And all properties mentioned before remain identical.
(It's even a little bit easier to build, as xxh_x86dispatch.c doesn't require any vector compilation flag anymore).

The `x86dispatch` PoC has been updated, employing the new strategy of starting from a baseline (typically `x64`+`sse2`) and adding vector instructions selectively. Everything seems to work fine. No more crash. And all properties mentioned before remain identical. (It's even a little bit easier to build, as `xxh_x86dispatch.c` doesn't require any vector compilation flag anymore).
easyaspi314 commented 2020-06-09 16:46:39 -05:00 (Migrated from github.com)

So I am wondering... Wouldn't it be easier to do the table method?

That means this;

// Top of file
#ifdef XXH_X86DISPATCH
static const XXH3_dispatch_table_t *XXH3_dispatchTable = NULL;
#endif

// For each implementation
static const XXH3_dispatch_table_t XXH3_dispatchTableAvx2 = {
  ... 
};
#ifndef XXH_X86DISPATCH
static const XXH3_dispatch_table_t *const XXH3_dispatchTable = &XXH3_dispatchTable_avx2;
#endif

That would abstract away the function pointers, as you can just do XXH3_dispatchTable->hashLong(...). It also fixes the DRY issue.

Additionally, you can just place a macro like

#ifdef XXH_X86DISPATCH
# define XXH_X86DISPATCH_CHECK \
do { \
    if (XXH3_dispatchTable == NULL) { \
       ... feature test ... \
    }
} while (0)
#else
# define XXH_X86DISPATCH_CHECK ((void)0)
#endif
So I am wondering... Wouldn't it be easier to do the table method? That means this; ```c // Top of file #ifdef XXH_X86DISPATCH static const XXH3_dispatch_table_t *XXH3_dispatchTable = NULL; #endif // For each implementation static const XXH3_dispatch_table_t XXH3_dispatchTableAvx2 = { ... }; #ifndef XXH_X86DISPATCH static const XXH3_dispatch_table_t *const XXH3_dispatchTable = &XXH3_dispatchTable_avx2; #endif ``` That would abstract away the function pointers, as you can just do `XXH3_dispatchTable->hashLong(...)`. It also fixes the DRY issue. Additionally, you can just place a macro like ```c #ifdef XXH_X86DISPATCH # define XXH_X86DISPATCH_CHECK \ do { \ if (XXH3_dispatchTable == NULL) { \ ... feature test ... \ } } while (0) #else # define XXH_X86DISPATCH_CHECK ((void)0) #endif ```
Cyan4973 commented 2020-06-09 17:14:22 -05:00 (Migrated from github.com)

I presume this could be helpful, yes, though the devil is in the details.
In particular, one goal is to preserve xxh3.h as much as possible.

I understand that repeating code is instinctively "bad", but keeping in mind that libxxhash is far more valuable than x86dispatch, one has to be cautious about the amount of changes that libxxhash can reasonably accommodate for the benefit of x86dispatch. Copying a few lines of code into x86dispatch is not the end of the world, and improving that situation should not come "at any cost".

But sure, this can be improved incrementally. Given it's a PoC, there's a lot to improve.
As an example, I'll likely consider introducing a structure of function pointers as you suggest, as opposed to N function pointers as it is done in the PoC now. This feels better.

I presume this could be helpful, yes, though the devil is in the details. In particular, one goal is to preserve `xxh3.h` as much as possible. I understand that repeating code is instinctively "bad", but keeping in mind that `libxxhash` is far more valuable than `x86dispatch`, one has to be cautious about the amount of changes that `libxxhash` can reasonably accommodate for the benefit of `x86dispatch`. Copying a few lines of code into `x86dispatch` is not the end of the world, and improving that situation should not come "at any cost". But sure, this can be improved incrementally. Given it's a PoC, there's a lot to improve. As an example, I'll likely consider introducing a structure of function pointers as you suggest, as opposed to N function pointers as it is done in the PoC now. This feels better.
Cyan4973 commented 2020-06-09 19:18:45 -05:00 (Migrated from github.com)

In branch x86dispatch_table, there's a new version of xxh_x86dispatch.c, which adds support for another function XXH3_64bits_withSecret_dispatch(), and uses a table of struct in order to group all variants into a const table of function pointers.
It simplifies the logic a bit, removing several lines of code.

As a side effect though, XXH_64bits_dispatch() loses some speed in AVX2 runtime, coming back to 50 GB/s (as before). Interestingly, XXH_64bits_withSeed_dispatch() doesn't, so it still runs at ~60 GB/s. This makes it faster than the regular variant !
I presume this speed impact is not directly related to the code, but is more likely a side effect of instruction alignment, which is more or less random from pure C (could be tamed with assembly though). Still, it's a surprisingly high impact.

In branch [x86dispatch_table](https://github.com/Cyan4973/xxHash/tree/x86dispatch_table), there's a new version of `xxh_x86dispatch.c`, which adds support for another function `XXH3_64bits_withSecret_dispatch()`, and uses a table of `struct` in order to group all variants into a `const` table of function pointers. It simplifies the logic a bit, removing several lines of code. As a side effect though, `XXH_64bits_dispatch()` loses some speed in AVX2 runtime, coming back to 50 GB/s (as before). Interestingly, `XXH_64bits_withSeed_dispatch()` doesn't, so it still runs at ~60 GB/s. This makes it faster than the regular variant ! I presume this speed impact is not _directly_ related to the code, but is more likely a side effect of instruction alignment, which is more or less random from pure C (could be tamed with assembly though). Still, it's a surprisingly high impact.
👍 1
gzm55 commented 2020-06-09 20:58:04 -05:00 (Migrated from github.com)

when compiled with -mavx512f, something in the detection routine makes the executable fail at runtime :
[1] 98930 illegal hardware instruction
This feels like an avx512 instruction is used on a cpu without avx512 support, which obviously doesn't end well. But it seems to happen inside XXH_featureTest(). Something to look at ...

maybe we should add __attribute__((__target__ ("sse2"))) for all non-dispatch functions when compiling with x86 runtime dispatching feature.

@Cyan4973 did we try -march=x86-64 -mtune=avx512f options to generate lean instructions for common functions?

> > when compiled with `-mavx512f`, something in the detection routine makes the executable fail at runtime : > > `[1] 98930 illegal hardware instruction` > > This feels like an `avx512` instruction is used on a cpu without `avx512` support, which obviously doesn't end well. But it seems to happen inside `XXH_featureTest()`. Something to look at ... > > maybe we should add `__attribute__((__target__ ("sse2")))` for all non-dispatch functions when compiling with x86 runtime dispatching feature. @Cyan4973 did we try `-march=x86-64 -mtune=avx512f` options to generate lean instructions for common functions?
easyaspi314 commented 2020-06-09 21:11:06 -05:00 (Migrated from github.com)

-mtune requires either a CPU name, vendor name, or generic.

I would consider -mtune=haswell, as haswell tunes are pretty generic (like core2), but it also works around the GCC AVX2 bug.

As for what __attribute__((target("X"))) does, it simply simulates that a function is compiled with -mX (with some restrictions).

The reason I recommend adding the SSE2 flag is that i386/i686 targets don't necessarily have SSE2. -march=i686 will (unless overridden by the target) compile for a Pentium II which lacks SSE2.

`-mtune` requires either a CPU name, vendor name, or `generic`. I would consider `-mtune=haswell`, as `haswell` tunes are pretty generic (like `core2`), but it also works around the GCC AVX2 bug. As for what `__attribute__((target("X")))` does, it simply simulates that a function is compiled with `-mX` (with some restrictions). The reason I recommend adding the SSE2 flag is that i386/i686 targets don't necessarily have SSE2. `-march=i686` will (unless overridden by the target) compile for a Pentium II which lacks SSE2.
👍 1
Cyan4973 commented 2020-06-10 12:46:46 -05:00 (Migrated from github.com)

The code in branch x86dispatch_table has been updated again, this time reducing the amount of source code dealing with small inputs.
All entry points coalesce into one generic function XXH_64bits_internal(), employing function pointers to split processing again on reaching the next stage, dealing with long inputs.
This reduces code repetition, and in a lesser way code size, as the effect is mitigated by the need to employ generic all-encompassing interfaces, which are then adapted, costing a few additional lines here and there.

I'm now looking for issues like this one : https://github.com/Cyan4973/xxHash/blob/x86dispatch_table/xxh3.h#L1788
where the source code presumes that it is being compiled for a single clear vector instruction set.
Unfortunately, that's no longer the case when using the dispatcher, since any mode, from scalar to avx512, could end up being selected at runtime.

I guess I'll have to settle for something simpler and more generic, even if it ends up being less optimized for a few targets.

The code in branch [x86dispatch_table](https://github.com/Cyan4973/xxHash/tree/x86dispatch_table) has been updated again, this time reducing the amount of source code dealing with small inputs. All entry points coalesce into one generic function `XXH_64bits_internal()`, employing function pointers to split processing again on reaching the next stage, dealing with long inputs. This reduces code repetition, and in a lesser way code size, as the effect is mitigated by the need to employ generic all-encompassing interfaces, which are then adapted, costing a few additional lines here and there. I'm now looking for issues like this one : https://github.com/Cyan4973/xxHash/blob/x86dispatch_table/xxh3.h#L1788 where the source code presumes that it is being compiled for a single clear vector instruction set. Unfortunately, that's no longer the case when using the dispatcher, since any mode, from `scalar` to `avx512`, could end up being selected at runtime. I guess I'll have to settle for something simpler and more generic, even if it ends up being less optimized for a few targets.
easyaspi314 commented 2020-06-10 13:26:07 -05:00 (Migrated from github.com)

That XXH3_64bits_internal also has the side effect of reducing code size on -Oz/-Os by a good 100+ bytes (albeit less on thumb)

Compiler Arch dev x86dispatch_table
clang 9.0.1 aarch64 8236 8048
gcc 9.3.0 aarch64 8352 8228
clang 9.0.1 x86_64 7904 7790
gcc 10.1.0 x86_64 7162 7059
clang 9.0.1 i686+SSE2 13289 13183
gcc 10.1.0 i686+SSE2 13861 13684
clang 9.0.1 armv7-a+thumb2 9576 9516
gcc 10.1.0 armv7-a+thumb2 10152 10016
That `XXH3_64bits_internal` also has the side effect of reducing code size on `-Oz`/`-Os` by a good 100+ bytes (albeit less on thumb) | Compiler | Arch | `dev` | `x86dispatch_table` | |:----|:-----|-----:|--------:| | clang 9.0.1 | aarch64 | 8236 | 8048 | | gcc 9.3.0 | aarch64 | 8352 | 8228 | | clang 9.0.1 | x86_64 | 7904 | 7790 | | gcc 10.1.0 | x86_64 | 7162 | 7059 | | clang 9.0.1 | i686+SSE2 | 13289 | 13183 | | gcc 10.1.0 | i686+SSE2 | 13861 | 13684 | | clang 9.0.1 | armv7-a+thumb2 | 9576 | 9516 | | gcc 10.1.0 | armv7-a+thumb2 | 10152 | 10016 |
👍 1
Cyan4973 commented 2020-06-11 22:35:00 -05:00 (Migrated from github.com)

Superseded by #387

Superseded by #387

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
announcement

   
bug

   
build

   
CI
Continuous Integration tests

  
dependencies
Pull requests that update a dependency file

  
documentation

   
duplicate

   
enhancement

   
experience feedback

   
feature request

   
github_actions
Pull requests that update GitHub Actions code

  
help wanted

   
install

   
invalid

   
Long-term

   
optimization

   
portability

   
question

   
target-specific
issue triggered in a specific environment (harware, compiler, OS)

  
test issue

   
wontfix
No labels
announcement
bug
build
CI
dependencies
documentation
duplicate
enhancement
experience feedback
feature request
github_actions
help wanted
install
invalid
Long-term
optimization
portability
question
target-specific
test issue
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!358
No description provided.