x86 CLMUL CRC rewrite #127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 11, 2024
This reverts commit 9f1a6d6.
It's not enough to silence the address sanitizer. Also memory and thread sanitizers would need to be silenced. They, at least currently, aren't smart enough to see that the extra bytes are discarded from the xmm registers by later instructions. Valgrind is smarter, possibly because this kind of code isn't weird to write in assembly. Agner Fog's optimizing_assembly.pdf even mentions this idea of doing an aligned read and then discarding the extra bytes. The sanitizers don't instrument assembly code but Valgrind checks all code. It's better to change the implementation to avoid the sanitization attributes which also look scary in the code. (Somehow they can look more scary than __asm__ which is implictly unsanitized.) See also: #112 #122
It was already commented out.
It's a standalone program that prints the required constants. It's won't be a part of the normal build of the package.
Now it refers to crc_clmul_consts_gen.c. vfold8 was renamed to mu_p and the p no longer has the lowest bit set (it makes no difference as the output bits it affects are ignored).
By using modulus scaled constants, the final reduction can be simplified.
This way it's clearer that two things cannot be selected at the same time.
Larhzu
force-pushed
the
crc_clmul_rewrite
branch
from
8931e99 to
888cef9
Compare
It's faster with both tiny and large buffers and doesn't require disabling any sanitizers. With large buffers the extra speed is from folding four 16-byte chunks in parallel. The 32-bit x86 with MSVC reportedly still needs a workaround. Now the simpler "__asm mov ebx, ebx" trick is enough but it needs to be in lzma_crc64() instead of crc64_arch_optimized(). Thanks to Iouri Kharon for testing and the fix. Thanks to Ilya Kurdyukov for testing the speed with aligned and unaligned buffers on a few x86 processors and on E2K v6. Thanks to Sam James for general feedback. Fixes: #112 Fixes: #122
On E2K the function compiles only due to compiler emulation but the function is never used. It's cleaner to omit the function when it's not needed even though it's a "static inline" function. Thanks to Ilya Kurdyukov.
Larhzu
force-pushed
the
crc_clmul_rewrite
branch
from
3427b26 to
30a2d5d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's faster with both tiny and large buffers and doesn't require disabling any sanitizers.
With large buffers the extra speed is from folding chunks in parallel. For large buffers, faster versions are out there but it's diminishing returns in context of XZ Utils. So let's skip wider folding or AVX2 code at least for now.