Do not pass -m flags when compiling shuffle.c

[mgorny]

Do not pass -msse2, -mavx2, etc. flags to the compiler when compiling shuffle.c. From what I can see, the file itself does not use any of these intrinsics, and they are only used by functions declared in bitshuffle-*.c and shuffle-*.c (where the respective flags are still passed). This prevents the compiler from incidentally optimizing the code called independenlty of the runtime CPU check to these instruction sets, effectively causing SIGILL on other CPUs.

I have verified that this fixes the issue on -march=znver2, but also does not cause any issues on -march=x86-64 and -march=i686.

Fixes #621

[FrancescAlted]

Thanks @mgorny for this. However, your pull request will not use AVX2 in machines having it (you can easily check that by putting a printf() in https://github.com/Blosc/c-blosc2/blob/main/blosc/shuffle.c#L312). This is because how the dynamic dispatching has been implemented inside blosc2; maybe there is a better way for implementing the dynamic dispatching, but again, your PR, as-is, is effectively disabling the use of AVX2.

Unfortunately AVX2 can greatly accelerate shuffle/unshuffle as well as bitshuffle/bitunshuffle, and it would be bad if users cannot leverage this capability anymore.

Also, @t20100 may have something to say here.

[mgorny]

Oh, I'm sorry about that. I see the problem now, the SHUFFLE_USE* macros are the problem. Apparently they were added in dd57c03, but the commit message doesn't really give an example of the problem they solved. In fact, they explicitly contradict CMakeLists.txt:

c-blosc2/blosc/CMakeLists.txt

Lines 345 to 347 in dc5d651

	# Define a symbol for the shuffle-dispatch implementation
	# so it knows AVX2 is supported even though that file is
	# compiled without AVX2 support (for portability).

FWICS top-level CMakeLists.txt has a pretty specific list of compilers supporting AVX2, etc., so I'm surprised they would need to be double-checked like that. Of course, it would be much cleaner to perform a compile check rather than hardcode a list of compilers.

[mgorny]

I've updated the pull request to fix the immediate issue. I can work on it further when I know what's the issue that prompted dd57c03.

[t20100]

This commit (dd57c03) was to add support of macos universal2 builds (#431, #430). The issue in this case is that one compilation command builds both ARM and x86, so source files cannot be added conditionally to the build, nor macro defined per architecture.

On option proposed in Blosc/c-blosc#347 (Same as #431 but for c-blosc1):

An alternative could be to define blosc_internal_*_avx2 and blosc_internal_*_sse2 functions even when AVX2/SSE2 are not defined with a body calling e.g., abort().

With this shuffle.c can be built without SIMD options and all source files can be built on all architectures.

[FrancescAlted]

Ok, the latest version of this PR is using AVX2 on my box, so that's great. @t20100 I am not 100% certain if you agree with this PR or if you foresee issues with universal2 builds. Can you clarify?

[t20100]

As it is, I would expect universal2 build to no longer work and any difference between what the CMakefile probes and SIMD macros like:

c-blosc2/blosc/bitshuffle-avx512.c

Line 24 in dc5d651

#if defined(__AVX512F__) && defined (__AVX512BW__)

to lead to issues.
I'll propose a change for this to clarify.

[t20100]

Hope this helps to clarify: t20100@b243896

It's only done for AVX2 and would need to be done for all SIMD implementations. It's maybe not necessary to do it for both shuffle and bitshuffle since the marco tests are the same.

It's basically adding a runtime test of the SIMD implementation availability and dummy functions when not available.
It still needs testing.

[mgorny]

Should I add these to this PR or are you going to take it from here?

[t20100]

Should I add these to this PR or are you going to take it from here?

As you want.
I first wanted to get feedbacks before going further.

If it's OK, then you can either get my changes or I can make a PR to your branch to add it to this PR.

[FrancescAlted]

@t20100 In general I like were you are headed (I just suggested to use _AVX2 suffix). It would be nice if both of you can contribute in this same PR (I don't if github makes this difficult though).

[mgorny]

Ok, I'll update the PR in a few minutes.

[mgorny]

@t20100 In general I like were you are headed (I just suggested to use _AVX2 suffix). It would be nice if both of you can contribute in this same PR (I don't if github makes this difficult though).

FWICS all the existing functions have _AVX suffix. Should I rename them?

[mgorny]

(same goes for SSE2 bits having SSE suffix)

[FrancescAlted]

@t20100 In general I like were you are headed (I just suggested to use _AVX2 suffix). It would be nice if both of you can contribute in this same PR (I don't if github makes this difficult though).

FWICS all the existing functions have _AVX suffix. Should I rename them?

Oh, I forgot about this. Yeah, as they are internal functions, I think renaming is a good idea.

[FrancescAlted]

(same goes for SSE2 bits having SSE suffix)

Let's do that here too.

[mgorny]

One more quick question: I see that some header files (but not all) declare additional functions such as bshuf_shuffle_bit_eightelem_altivec() that aren't used elsewhere. Should I remove them from the headers while at it?

[FrancescAlted]

One more quick question: I see that some header files (but not all) declare additional functions such as bshuf_shuffle_bit_eightelem_altivec() that aren't used elsewhere. Should I remove them from the headers while at it?

Hmm, I'd say it is probably safe to remove those functions from the header, but I am not completely sure. Perhaps @kif can shed some light here?

[FrancescAlted]

One more quick question: I see that some header files (but not all) declare additional functions such as bshuf_shuffle_bit_eightelem_altivec() that aren't used elsewhere. Should I remove them from the headers while at it?

Hmm, I'd say it is probably safe to remove those functions from the header, but I am not completely sure. Perhaps @kif can shed some light here?

Thinking twice about this, let's focus in our problem at hand, and let the cleanup to happen in another PR.

[mgorny]

Well, unfortunately this brings the second question: since these functions are technically present in the header, should I also stub them with abort()s? To be honest, that's some extra work that I don't really want to go through.

My rough idea for this PR would be to, in order:

1. Remove unneeded function decls from headers.
2. Rename functions for consistency.
3. Add checks and stubs as requested by @t20100.
4. And then, when everything's ready, remove the flags as this PR currently does.

[FrancescAlted]

Well, unfortunately this brings the second question: since these functions are technically present in the header, should I also stub them with abort()s? To be honest, that's some extra work that I don't really want to go through.

My rough idea for this PR would be to, in order:

1. Remove unneeded function decls from headers.

2. Rename functions for consistency.

3. Add checks and stubs as requested by @t20100.

4. And then, when everything's ready, remove the flags as this PR currently does.

I see. I'm +1 on your plan then.

[mgorny]

Actually, I was wrong. Some of these functions are used cross-file, e.g. AVX512 uses AVX2 and SSE2 functions.

[mgorny]

@t20100, actually, could you do the part with guards? I'm entirely unfamiliar with this code, and I'm getting too easily distracted for this.

[t20100]

OK, I'll take care of the guards part.

[t20100]

I made a PR on this branch with stubs when SIMD are not available and runtime availability checks: mgorny#1

What do you think?

[mgorny]

Thanks a lot! I've merged it here.

[FrancescAlted]

Hi. I have tried this out, and it looks good to me. If there is no opposition, I'll merge this soon. Thanks @mgorny and @t20100 !

[mgorny]

Thank you!