Closed parheliamm closed 8 months ago
I created a branch memcmplen_arm64 which should do the same as your first commit and also adds MSVC support on ARM64 (untested).
The commit message of your second commit has significantly higher numbers on the memcpy line. I'm not familiar with lzbench but I wonder if 10 % difference in memcpy could indicate that there was something different on the test computer and thus the benchmark results could be slightly different too. The difference in compression speed is small so it would be good to be sure that it's not due to noise.
In any case, the second patch cannot be accepted. Unfortunately you have misunderstood the problem with type punning. It's about the C programming language and how modern compilers optimize while still staying within the exact requirements of the C standard. Unsafe use of type punning breaks strict aliasing rules and might result in broken executables. The instruction set being used doesn't matter; even if unaligned access wasn't supported by the hardware, type punning would be problematic with modern compilers when accessing aligned data.
The memcpy method used by tuklib_integer.h should compile to a single instruction with modern GCC and Clang/LLVM versions when building for a target that supports fast unaligned access. Thus the use of type punning shouldn't make a difference on ARM64. However, it's possible that compilers do something slightly differently still and thus there could be a difference in practice, or the violation of aliasing rules allows compilers to do something that happens to work but could cause problems some day. It's a bit annoying situation but I don't know any better way.
Thanks!
I created a branch memcmplen_arm64 which should do the same as your first commit and also adds MSVC support on ARM64 (untested).
The commit message of your second commit has significantly higher numbers on the memcpy line. I'm not familiar with lzbench but I wonder if 10 % difference in memcpy could indicate that there was something different on the test computer and thus the benchmark results could be slightly different too. The difference in compression speed is small so it would be good to be sure that it's not due to noise.
In any case, the second patch cannot be accepted. Unfortunately you have misunderstood the problem with type punning. It's about the C programming language and how modern compilers optimize while still staying within the exact requirements of the C standard. Unsafe use of type punning breaks strict aliasing rules and might result in broken executables. The instruction set being used doesn't matter; even if unaligned access wasn't supported by the hardware, type punning would be problematic with modern compilers when accessing aligned data.
The memcpy method used by tuklib_integer.h should compile to a single instruction with modern GCC and Clang/LLVM versions when building for a target that supports fast unaligned access. Thus the use of type punning shouldn't make a difference on ARM64. However, it's possible that compilers do something slightly differently still and thus there could be a difference in practice, or the violation of aliasing rules allows compilers to do something that happens to work but could cause problems some day. It's a bit annoying situation but I don't know any better way.
Yes, you are correct, I disassemble the code, not only memcpy but also UNSAFE_TYPE_PUNNING are all interpreted as below:
640: f9400000 ldr x0, [x0]
644: d65f03c0 ret
So the 2nd patch is useless, we can keep the 1st patch only.
Thanks!
The 8-byte version is now enabled in memcmplen.h for ARM64 in the master branch. It will be included in XZ Utils 5.6.0. Thanks!
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces: - [ ] Bugfix - [ ] Feature - [ ] Code style update (formatting, renaming, typo fix) - [ ] Refactoring (no functional changes, no api changes) - [x] Build related changes - [ ] Documentation content changes - [x] Other (please describe): ## What is the current behavior?Related Issue URL:
What is the new behavior?
select enable-unsafe-type-punning if enable_unaligned_access enabled
Does this introduce a breaking change?
Other information
Performance improvements on ARM64 with 2 commits:
Patched: