[PATCH] D46163: [DO NOT SUBMIT] Do strlen() while computing xxhash
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Apr 28 10:56:58 PDT 2018
ruiu added inline comments.
================
Comment at: llvm/lib/Support/xxhash.cpp:148
+ const uint64_t *R = (const uint64_t *)Q;
+ if (uint64_t X = (R[0] - 0x0101010101010101) & ~R[0] & 0x8080808080808080) {
+ Remaining = (__builtin_ffsl(X) >> 3) - 1;
----------------
grimar wrote:
> btw, if you pass P=="ab\0" to this method,
>
> then `R[0]` will read the rest 5 bytes of memory from out of bounds, no?
That's intentional.
================
Comment at: llvm/lib/Support/xxhash.cpp:149
+ if (uint64_t X = (R[0] - 0x0101010101010101) & ~R[0] & 0x8080808080808080) {
+ Remaining = (__builtin_ffsl(X) >> 3) - 1;
+ break;
----------------
grimar wrote:
> I tried to build this patch with MSVS, but it complains about the absence of this function.
>
> I had to use the following instead:
>
> ```
> int psnip_builtin_ffsl(long v) {
> unsigned long r;
> if (_BitScanForward(&r, (unsigned long)v)) {
> return (int)(r + 1);
> }
> return 0;
> }
> ```
> (took it from https://github.com/nemequ/portable-snippets/tree/master/builtin).
>
> But it does not work correctly it seems. I am testing on `lld-speed-test\lld-speed-test\scylla`,
> here at some point `Remaining` becomes (-1). And then lines below (185, 186)
>
> ```
> const char *End = Q + Remaining;
> size_t Len = End - P;
> ```
>
> handle it wrong and code ends up with
> an infinite loop in `splitStrings1` because `strlen_xxHash64` returns -1 always.
>
>
I'm not very familiar with these intrinsics of MSVC, but I believe you forgot to use the 64 bit version. Use this.
static inline int my_ffsl(uint64_t v) {
uint64r_t R;
_BitScanForward64(&R, V);
return R;
}
Note that this should be compiled to a single BSF instruction on x86-64.
https://reviews.llvm.org/D46163
More information about the llvm-commits
mailing list