XXH_ASSUME macro using __builtin_assume if supported (clang only fo…

[devnexen]

…r now).

[t-mat]

This my comment is not for this PR itself. But for recording and future investigation.

---

CI test fails for clang-5, 32-bits. But all other versions of clangs are OK.

https://github.com/Cyan4973/xxHash/actions/runs/4271811509/jobs/7438606909#step:13:636

---- test 32-bit ----
./xxhsum32 -bi0 xxhash.c
                                                                      
xxhsum32 0.8.1 by Yann Collet 
compiled as 32-bit i386 + SSE2 little endian with Clang 5.0.1 (tags/RELEASE_501/final) 

Error: 32-bit hash test 19: Internal sanity check failed!

Got 0xFFDD35B6, expected 0x5BD11DBD.

[Cyan4973]

Yes, I noted that after reviewing the code.
There is no such issue with the dev branch, so the logical conclusion is that the problem is triggered by this PR.

That's weird, because the PR content looks straightforward.
So maybe it's a second order issue.
For example, maybe there is an incorrect assume somewhere ...

Note :
the failing test is xxh32, compiled in 32-bit mode, running on an input of length 222.
There might be other failures, it's just the first one that fails, and the sanity check stops right there.
Unfortunately, I don't have clang-5 on my local system to test, the oldest variant I can reach is clang-6.0, and it works fine, so I can't reproduce the issue at this point.

There is no XXH_ASSUME employed directly in the code. Instead, XXH_ASSERT() becomes XXH_ASSUME() in release mode.

But xxh32 is particularly light in term of assert and therefore assume. There are only a few occurrences, and they seem uncontroversial. Weird ...

[devnexen]

Note that this builtin is supported from clang 15.

[Cyan4973]

Note that this builtin is supported from clang 15.

In which case XXH_HAS_BUILTIN(__builtin_assume) should return 0 ?
Therefore, XXH_ASSUME should remain strictly identical with this PR ?

I like this scenario, though it doesn't explain why the sanity test fails on clang-5.

[devnexen]

With this basic Dockerfile

FROM i386/ubuntu:bionic
RUN apt-get update && apt-get --fix-missing install -y --no-install-recommends clang-5.0 g++ git make
COPY xxhash xxhash
RUN cd xxhash && make CC=clang-5.0 CXX=clang++-5.0 CFLAGS=-fPIC CXXFLAGS=-fPIC clean test-all

it seems to pass for me.

[Cyan4973]

The really strange part is that, having repeated this test multiple times on Github Actions, it fails reliably, at the same position, every time.
This rules out the possibility of a random hardware issue.

Considering the nature of the code change, it makes no sense for it to have any impact on clang-5.0 specifically.

[devnexen]

True. For the life of me, I cannot wrap my head around it even within docker it passes...

[Cyan4973]

With this basic Dockerfile

FROM i386/ubuntu:bionic
RUN apt-get update && apt-get --fix-missing install -y --no-install-recommends clang-5.0 g++ git make
COPY xxhash xxhash
RUN cd xxhash && make CC=clang-5.0 CXX=clang++-5.0 CFLAGS=-fPIC CXXFLAGS=-fPIC clean test-all

it seems to pass for me.

On a related note, I tried a similar docker test on a local Ubuntu station,
and it also works fine.

That doesn't explain why it fails on Github Actions specifically,
but the significance of this failure becomes more questionable.

[Cyan4973]

I'll go ahead and pretend that this test failure is not significant, and not reproduced.

This is helped by the fact that we just dropped Ubuntu18 support in Github Actions, because it's no longer supported,
this will indirectly stop clang-5.0 tests, so this failure signal will not reproduce on future dev tests.

The PR itself is very clear, and there is really no reason for it to produce such a side effect.