License, Windows and Compatibility Upgrade #18

neurolabusc · 2020-01-28T15:42:35Z

This pull request adds 3 benefits:

The GPL code for SIMD CRC has been replaced by BSD-license code from the Chrome browser. Performance is identical, as demonstrated in this minimal project. This addresses complications raised with GPL code.
At run-time, the software now detects if the host computer supports CLMUL instructions, using the accelerated CRC if supported. This prevents the software from executing illegal instructions on hardware that does not support CLMUL or where CLMUL has been disabled by a OEM BIOS or virtual machine. The code for detection is designed to be backward compatible with old compilers (e.g. gcc 4.8.4) that report to CMAKE that they support CLMUL instructions but do not provide the cpuid detection of CLMUL.
The CMAKE files have been updated to support modern Microsoft compilers allowing simpler distribution to those using the Windows operating system.

A benchmark demonstrates that this code is robust and outperforms the baseline and zlib-ng on a wide range of hardware and operating systems.

I would be excited to see this pull request included, as it can be called from the pigz make. This dramatically accelerates pigz, with the SIMD CRC directly accelerating the serial stage that is most impacted by Amdahl's law.

…ttps://github.com/neurolabusc/simd_crc

…rtium/ITK#416)

…ei/zlib)

Adenilson · 2020-01-28T20:05:41Z

crc32.c

 #ifdef HAS_PCLMUL

+//https://github.com/webosose/chromium68/blob/master/src/third_party/zlib/crc32_simd.c


Maybe instead of pointing to an outdated copy of Chromium (M68), you could reference the ToT (Top of Tree) original repository Chromium repo*?

Since M68, I've added some new optimizations, refactorings and bug fixes on Chromium's zlib.
:-)

*https://cs.chromium.org/chromium/src/third_party/zlib/

Adenilson · 2020-01-28T20:08:44Z

crc32.c

@@ -21,6 +21,13 @@
  DYNAMIC_CRC_TABLE and MAKECRCH can be #defined to write out crc32.h.
 */

+#ifdef HAS_PCLMUL


I suggest you try to follow the integration method used in Chromium's zlib i.e. keeping SIMD/optimizations in separated files.

@Adenilson I have done so in my fork. Can I suggest I withdraw this pull request, and you either comment on my fork or make your own fork. Once all your concerns are addressed, we can generate a new pull request to the CloudFlare repo. I have tested the separate files on both MacOS and Linux, so I think this addresses your concern.

@Adenilson I should also point out that my repo copies the Chromium zlib code verbatim, and still includes the ARM code, even though I only tested and included the x86 code. I think the whole community would benefit if the ARM code was enhanced, but that is outside my wheelhouse.

vkrasnov · 2020-01-29T00:14:03Z

crc32.c

+#ifdef HAS_PCLMUL
+ #include <emmintrin.h>
+ #include <smmintrin.h>
+ #include <wmmintrin.h>


I think all of those can be replaced by a single #include <immintrin.h>

defines no longer in crc32, @Adenilson can decide if these can be replaced in arc_simd.c

vkrasnov · 2020-01-29T00:15:07Z

crc32.c

 #ifdef _MSC_VER
 #define zalign(x) __declspec(align(x))
 #else
 #define zalign(x) __attribute__((aligned((x))))
 #endif

-uLong crc32_simd(unsigned char const *buf, size_t len, uInt crc) {
+uint crc32_simd(unsigned char const *buf, size_t len, uInt crc) {


I would rather use uint32_t everywhere the size is known to be 32 bits.

fixed (rordenlab@59c0c27)

vkrasnov · 2020-01-29T00:17:50Z

crc32.c

 #define PCLMUL_MIN_LEN 64
 #define PCLMUL_ALIGN 16
 #define PCLMUL_ALIGN_MASK 15

+int cpu_has_pclmul = -1; //global: will be 0 or 1 after first test
+
+int has_pclmul(void) {


Could this perhaps be called at init time with __attribute__((constructor)) ?

@vkrasnov I think my solution is robust and efficient and works on all the compilers I tested it on (gcc 4.8..9.2; Clang 10, Microsoft C compiler). No other location in this library uses the __attribute__((constructor)) style, and it is unclear if it is universally accepted. This is your code, which you maintain, so ultimately should be familiar to you. Since that style is unfamiliar to me, I would be grateful if you or @Adenilson could devise the implementation you are happy with.

I'm not sure about __attribute__((constructor)), e.g. Rust intentionally avoids life before main, so I'm not sure how that would work if this library was wrapped as a Rust crate.

However, unsychronized access to a global variable is Undefined Behavior if code runs on multiple threads, and we definitely will use it in multi-threaded programs. So at very least it should be marked as atomic. I know x86 memory model is forgiving about such things, but things that are UB in the compiler, but happen to work in practice are a thin ice.

@kornelski -I have tried to address this in my fork. The value is atomic. There is also no a compiler flag SKIP_CPUID_CHECK that allows one to assume that the CPU supports PCLMUL. One option would be to make the default behavior be to assume PCLMUL support. This would my commit would have the same compatibility as the current code, simply using the Chromium BSD-licensed SIMD CRC in place of the GPL code. This would eliminate @Adenilson's performance concerns, albeit just like the current code it will fail if the code is compiled on a system that supports PCLMUL but is run on a computer without support.

Adenilson · 2020-01-31T06:04:06Z

@neurolabusc A question: how are you benchmarking? And with which data corpus?

neurolabusc · 2020-01-31T14:38:56Z

@Adenilson here is my benchmark for zlib and pigz. I explicitly test NIfTI-format images that are popular in my field. Tools like AFNI and FSL will save gzip-compressed images at each stage of processing, and individual files are often ~2Gb each (e.g. multi-band fMRI session).

I understand that @vkrasnov's original use case was a huge number of very small files. In my case, the penalty for a single cpuid call is negligible, while it may be more of an issue for other cases. One option is to simply have a compile flag to determine if the cpuid should be checked. People like @vkrasnov who build for modern facilities can compile to without supporting older processors (just like previous versions of this repository), while scientists may want to compile a version that does not crash if run on old hardware. I am open for any suggestions.

…/zlib#25)

neurolabusc · 2020-02-08T14:45:20Z

@Adenilson are you interested in implementing your changes to the CloudFlare zlib? Your changes cover the same ground as mine (remove GPL, aid MSVC updates), but is superior to mine with ARM support. Also, you provide a CPU detection that you suggest might be superior to mine. I am reluctant to put your changes into my fork, as I am not familiar with ARM.

If you will devote time to this, I will withdraw my PR. Once your PR is included, I can work to ensure the cmake supports MSVC.

On the other hand, if you are not interested in this, I would like to go ahead and submit a revised version of my own CloudFlare pull request. While it does not include all the features of yours, it is still superior in every respect to the current CloudFlare implementation.

Just tell me how to proceed. I can completely understand if you do not want to devote time to this. From my perspective, CloudFlare zlib includes benefits beyond the enhanced CRC discussed here, both for serial as well as parallel compression.

Adenilson · 2020-02-10T18:31:48Z

@neurolabusc I'm afraid I lack the bandwidth to dedicate to upstreaming changes to Cloudflare fork.

Maintaining the zlib used by Chrome and Android and trying to upstream changes to Canonical zlib is already a bit taxing.

* Replace GPL CRC with BSD CRC (zlib-ng/zlib-ng#42), for validation see https://github.com/neurolabusc/simd_crc * Westmere detection * Update configure for Westmere (InsightSoftwareConsortium/ITK#416) * use cpu_has_pclmul() to autodetect CPU hardware (InsightSoftwareConsortium/ITK#416) * remove gpl code * Improve support for compiling using Windows (https://github.com/ningfei/zlib) * Import ucm.cmake from https://github.com/ningfei/zlib * crc32_simd as separate file (#18) * atomic and SKIP_CPUID_CHECK (#18) * Remove unused code * Only allow compiler to use clmul instructions to crc_simd unit (intel/zlib#25) * Atomic does not compile on Ubuntu 14.04 * Allow "cmake -DSKIP_CPUID_CHECK=ON .." to not check SIMD CRC support on execution. * Restore Windows compilation using "nmake -f win32\Makefile.msc" * zconf.h required for Windows nmake * support crc intrinsics for ARM CPUs * Requested changes (#19)

* Fix architecture macro on MSVC. Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Update '.gitignore'. Also remove 'zconf.h', it will be generated during building. * Add 'build' subdir to .gitignore * Fix architecture macro on MSVC. * Update CMakeLists.txt Clean up. Add option to set runtime (useful for MSVC). Add 'SSE4.2' and 'AVX' option. Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Define '_LARGEFILE64_SOURCE' by default in CMakeLists.txt. Also remove checking fseeko. Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Set 'CMAKE_MACOSX_RPATH' to TRUE. Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Update SSE4.2 and PCLMUL setting in CMakeLists.txt Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Add 'HAVE_HIDDEN' to CMakeLists.txt Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Fix building dll on MSVC. Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Fix zlib.pc file generation. Signed-off-by: Ningfei Li <ningfei.li@gmail.com> * Refine cmake settings. * Change cmake function to lowercase. * Fix zlib.pc * Fix crc32_pclmul_le_16 type. * Set POSITION_INDEPENDENT_CODE flag to ON by default. * Replace GPL CRC with BSD CRC (zlib-ng/zlib-ng#42), for validation see https://github.com/neurolabusc/simd_crc * Westmere detection * Update configure for Westmere (InsightSoftwareConsortium/ITK#416) * use cpu_has_pclmul() to autodetect CPU hardware (InsightSoftwareConsortium/ITK#416) * remove gpl code * Improve support for compiling using Windows (https://github.com/ningfei/zlib) * Import ucm.cmake from https://github.com/ningfei/zlib * crc32_simd as separate file (#18) * atomic and SKIP_CPUID_CHECK (#18) * Suppress some MSVC warnings. * Remove unused code * Fix ucm_set_runtime when only C is enabled for the project. * Removed configured header file. * Unify zconf.h template. * Only allow compiler to use clmul instructions to crc_simd unit (intel/zlib#25) * Atomic does not compile on Ubuntu 14.04 * Allow "cmake -DSKIP_CPUID_CHECK=ON .." to not check SIMD CRC support on execution. * Restore Windows compilation using "nmake -f win32\Makefile.msc" * Do not set SOVERSION on Cygwin. * Fix file permission. * Refine PCLMUL CMake option. * zconf.h required for Windows nmake * Do not set visibility flag on Cygwin. * Fix compiling dll resource. * Fix compiling using MinGW. SSE4.2 and PCLMUL are also supported. * Fix zconf.h for Windows. * support crc intrinsics for ARM CPUs * Add gzip -k option to minigzip (to aid benchmarks) * Support Intel and ARMv8 optimization in CMake. * Clean up. * Fix compiling on Apple M1. check_c_compiler_flag detects SSE2, SSE3, SSE42 and PCLMUL but compiling fails. Workaround fix, need further investigation. * Restore MSVC warnings. Co-authored-by: neurolabusc <rorden@sc.edu> Co-authored-by: Chris Rorden <rorden@mailbox.sc.edu>

See https://github.com/tobie/pr-preview/.

Followup to cloudflare#18

neurolabusc added 7 commits January 23, 2020 18:44

Replace GPL CRC with BSD CRC (zlib-ng/zlib-ng#42), for validation see h…

aa1deea

…ttps://github.com/neurolabusc/simd_crc

Westmere detection

1f5a420

Update configure for Westmere (InsightSoftwareConsortium/ITK#416)

2937f89

use cpu_has_pclmul() to autodetect CPU hardware (InsightSoftwareConso…

dd3e924

…rtium/ITK#416)

remove gpl code

b3064e0

Improve support for compiling using Windows (https://github.com/ningf…

4c77bc7

…ei/zlib)

Import ucm.cmake from https://github.com/ningfei/zlib

4bf55a2

neurolabusc mentioned this pull request Jan 28, 2020

Adding CPU features detection code madler/zlib#468

Open

neurolabusc requested a review from vkrasnov January 28, 2020 16:33

Adenilson reviewed Jan 28, 2020

View reviewed changes

vkrasnov reviewed Jan 29, 2020

View reviewed changes

vkrasnov requested review from kornelski and kentonv January 29, 2020 00:21

crc32_simd as separate file (cloudflare#18)

59c0c27

neurolabusc added 3 commits January 31, 2020 12:53

atomic and SKIP_CPUID_CHECK (cloudflare#18)

dc28f91

Remove unused code

b73befc

Only allow compiler to use clmul instructions to crc_simd unit (intel…

df5c4fe

…/zlib#25)

Atomic does not compile on Ubuntu 14.04

c99aa75

neurolabusc closed this Feb 14, 2020

neurolabusc mentioned this pull request Feb 14, 2020

License, Windows and Compatibility Upgrade (redux) #19

Merged

fhanau pushed a commit to fhanau/zlib that referenced this pull request Feb 27, 2023

Meta: add PR preview functionality (cloudflare#18)

a15e765

See https://github.com/tobie/pr-preview/.

fhanau pushed a commit to fhanau/zlib that referenced this pull request Feb 27, 2023

Rename .pr-preview to .pr-preview.json (cloudflare#20)

1fdea48

Followup to cloudflare#18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

License, Windows and Compatibility Upgrade #18

License, Windows and Compatibility Upgrade #18

neurolabusc commented Jan 28, 2020

Adenilson Jan 28, 2020

Adenilson Jan 28, 2020

neurolabusc Jan 29, 2020

neurolabusc Jan 29, 2020

vkrasnov Jan 29, 2020

neurolabusc Jan 29, 2020

vkrasnov Jan 29, 2020

neurolabusc Jan 29, 2020

vkrasnov Jan 29, 2020

neurolabusc Jan 29, 2020

kornelski Jan 31, 2020

neurolabusc Jan 31, 2020

Adenilson commented Jan 31, 2020

neurolabusc commented Jan 31, 2020

neurolabusc commented Feb 8, 2020

Adenilson commented Feb 10, 2020

		#ifdef HAS_PCLMUL

		//https://github.com/webosose/chromium68/blob/master/src/third_party/zlib/crc32_simd.c

License, Windows and Compatibility Upgrade #18

License, Windows and Compatibility Upgrade #18

Conversation

neurolabusc commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adenilson commented Jan 31, 2020

neurolabusc commented Jan 31, 2020

neurolabusc commented Feb 8, 2020

Adenilson commented Feb 10, 2020