Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (cf89cf3) 83.90% compared to head (30a6204) 83.89%.

@@             Coverage Diff             @@
##           develop    #1531      +/-   ##
===========================================
- Coverage    83.90%   83.89%   -0.01%     
===========================================
  Files          132      132              
  Lines        10842    10842              
  Branches      2793     2793              
===========================================
- Hits          9097     9096       -1     
- Misses        1046     1047       +1     
  Partials       699      699              

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

I think I'd prefer it if configure was changed to appending native instead of replacing the instruction set enabling compiler parameters. That would make it behave closer to the same as cmake and at the same time fix this problem.
IMO, that would be a cleaner solution, with the small benefit of still including optimizations for better instruction sets (in case of future cpu upgrades), and only setting a lower bound to equal the compiling cpus instruction sets.

And as an aside to this; The whole native support could be improved to possibly even bypass functable and such at some point, but that is a pretty big and invasive change.

I think I'd prefer it if configure was changed to appending native instead of replacing the instruction set enabling compiler parameters. That would make it behave closer to the same as cmake and at the same time fix this problem.

That wouldn't fix this specific issue which is basically caused by oversimplified preprocessor defines for SSE4 tests... Basically compiler can use inline assembly for SSE4, but our code has no inline assembly for SSSE3 instructions or older.

So the code itself only actually uses ssse3 instructions, but it's intentionally gated to use sse4.2 because the checksum + copy routine is actually slower on pre sse4.2 CPUs due to the misalignment penalty prior to nehalem. The ssse3 checksum version is the same but it does aligning scalar sums before doing purely aligned loads. The copy is then handled separately.

Ah, there are two different issues then.

But I still think this one is a configure/cmake issue. There are no cpus supporting SSE4.2 that do not also support SSSE3, and enabling SSE4.2 (-msse42 in GCC) will also enable all its predecessors. So how can configure then say that SSE4.2 is available, but SSSE3 is not?

I think this part of configure should be changed, as it is kind of backwards today:
https://github.com/zlib-ng/zlib-ng/blob/develop/configure#L1609

            if test ${HAVE_SSE42CRC_INTRIN} -eq 1 || test ${HAVE_SSE42CRC_INLINE_ASM} -eq 1; then
                CFLAGS="${CFLAGS} -DX86_SSE42"
                SFLAGS="${SFLAGS} -DX86_SSE42"

                if test ${HAVE_SSE42CRC_INTRIN} -eq 1; then
                  CFLAGS="${CFLAGS} -DX86_SSE42_CRC_INTRIN"
                  SFLAGS="${SFLAGS} -DX86_SSE42_CRC_INTRIN"
                fi

I think that code snippet should instead set: X86_SSE42 if intrinsics are available, and X86_SSE42_CRC_ASM if inline asm is supported. Perhaps also skip the asm fallback define entirely if intrinsics are supported?
This requires some minor changes to the ifdef checks of course, but IMO this would be the more logical way to do this. The intrinsics are after all the primary, and inline asm is the fallback. This is also how all the other instruction set defines are set (for x86 at least), this one is the odd one out, easily causing these kinds of weird issues and confusions.

So the code itself only actually uses ssse3 instructions, but it's intentionally gated to use sse4.2 because the checksum + copy routine is actually slower on pre sse4.2 CPUs due to the misalignment penalty prior to nehalem. The ssse3 checksum version is the same but it does aligning scalar sums before doing purely aligned loads. The copy is then handled separately.

This would be a great comment to add to the code actually.

Ah, there are two different issues then.

But I still think this one is a configure/cmake issue. There are no cpus supporting SSE4.2 that do not also support SSSE3, and enabling SSE4.2 (-msse42 in GCC) will also enable all its predecessors. So how can configure then say that SSE4.2 is available, but SSSE3 is not?

This isn't about supporting real CPUs... This is only about working around broken cpuid instruction in virtualized Docker containers. As much as I'd like to have best possible patch for everything, I'm really against making huge rewrite if there is shorter solution. The inline assembly was added to work around older compilers that didn't have support for CRC intrinsics but could target SSE4. There is still a lot of systems that are stuck using older compilers due to bugs in newer versions that only affect certain processors that we officially still support, but gcc for example doesn't.

I know this is not about cpu support, this is about the way we do detection, and for SSE4.2 that is done in a strange way that in this case breaks builds and assumptions. I don't see this as being a huge rewrite, it is rather a small number of defines that need renaming and a few ifdef checks that need to be changed. It would in fact improve code quality.
If you don't want to have a go at it, i'll try to find some time for it in the next couple of days, still have a bunch of RL backlog after 2 weeks on vacation.

If you don't want to have a go at it, i'll try to find some time for it in the next couple of days, still have a bunch of RL backlog after 2 weeks on vacation.

I have a real job to focus on currently... When I joined zlib-ng originally, I had been unemployed for over 10 years, so any project was good to keep my skills up-to-date. I still intend to make small patches once in a while, but I leave bigger ones to people who have more spare time.

This would be a great comment to add to the code actually.

I can make a pull request for that if it'd add clarity. I think the commit messages said something of the same effect.

This is not needed after #1542