Drop -msse4 compiler flag #27121

stanhu · 2021-08-25T07:38:53Z

Older CPUs that do not have SSE4.1 would crash with the Ruby native gem due to an illegal instruction exception.

The Abseil random library isn't being used at the moment (#26476), and there's no reason gRPC needs to force SSE4.1 instructions on all platforms at the moment. There are other hardware-specific issues that need to be ironed out for this to work: #26479

When the -msse4 compiler flag was enabled, the Abseil code started using the pinsrb instruction:

$ elfx86exts abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.o
MODE64 (ret)
CMOV (cmovne)
SSE2 (movdqa)
SSE41 (pinsrb)
SSE1 (movaps)
CPU Generation: Penryn

This was previously needed because gcc 4.8 wouldn't compile without the -msse4 and -maes flags.

However, per gcc-mirror/gcc@97db2bf gcc 5.0+ automatically detects whether these options are enabled.

clang still needs -maes since including wmmintrin.h expects the AES option to be enabled.

Closes #27095

@markdroth @veblush

Older CPUs that do not have SSE4.1 would crash with the Ruby native gem due to an illegal instruction exception. The Abseil random library isn't being used at the moment (grpc#26476), and there's no reason gRPC needs to force SSE4.1 instructions on all platforms at the moment. There are other hardware-specific issues that need to be ironed out for this to work: grpc#27121 When the `-msse4` compiler flag was enabled, the Abseil code started using the `pinsrb` instruction: ``` $ elfx86exts abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.o MODE64 (ret) CMOV (cmovne) SSE2 (movdqa) SSE41 (pinsrb) SSE1 (movaps) CPU Generation: Penryn ``` Closes grpc#27095

Older CPUs that do not have SSE4.1 would crash with the Ruby native gem due to an illegal instruction exception. The Abseil random library isn't being used at the moment (grpc#26476), and there's no reason gRPC needs to force SSE4.1 instructions on all platforms at the moment. There are other hardware-specific issues that need to be ironed out for this to work: grpc#26479 When the `-msse4` compiler flag was enabled, the Abseil code started using the `pinsrb` instruction: ``` $ elfx86exts abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.o MODE64 (ret) CMOV (cmovne) SSE2 (movdqa) SSE41 (pinsrb) SSE1 (movaps) CPU Generation: Penryn ``` Closes grpc#27095

mttkay · 2021-09-01T13:55:28Z

@veblush Just checking if you had a chance to look at this yet? Unfortunately our Ruby 3 migration is blocked by this issue, since we had to revert to an older version of gRPC (from times prior to gRPC receiving Ruby 3 related fixes). The only alternative would be for us to roll our own version of gRPC (or at least abseil-cpp) with these patches applied, which is a lot of effort.

stanhu · 2021-09-07T06:57:31Z

@jtattermusch Would you mind taking a look at this pull request?

veblush · 2021-09-08T23:03:52Z

AFAIK, those flags were necessary to build Abseil. But I could be wrong so let's check the result of CI.

FYI, I'm about to change how to build Abseil (#27193) so this will be solved once gRPC gets the next Abseil release. (presumably next this year)

stanhu · 2021-09-09T15:28:49Z

@veblush Looking at #23357, I see third_party/abseil-cpp/absl/random/internal/randen_hwaes.cc has this:

#elif defined(ABSL_ARCH_X86_64) || defined(ABSL_ARCH_X86_32)
// On x86 we rely on the aesni instructions
#include <wmmintrin.h>

This sounds like -maes might be necessary, but -msse4 could be dropped. What do you think?

stanhu · 2021-09-09T15:41:41Z

I'm not sure what is defining ABSL_ARCH_X86_64 because it's not being defined in my build. I added some garbage in the elif block, and gRPC compiled fine.

stanhu · 2021-09-09T19:31:07Z

Oh, it looks like abseil defines this: https://github.com/abseil/abseil-cpp/blob/997aaf3a28308eba1b9156aa35ab7bca9688e9f6/absl/random/internal/platform.h#L63

stanhu · 2021-09-09T19:40:12Z

It looks gcc 5 and up don't need these compiler flags because they are only enabled when specific flags are in use (gcc-mirror/gcc@97db2bf). For example:

#ifndef _WMMINTRIN_H_INCLUDED
#define _WMMINTRIN_H_INCLUDED

/* We need definitions from the SSE2 header file.  */
#include <emmintrin.h>

/* AES */

#if !defined(__AES__) || !defined(__SSE2__)
#pragma GCC push_options
#pragma GCC target("aes,sse2")
#define __DISABLE_AES__
#endif /* __AES__ */

emmintrin.h does this as well:

#ifndef _EMMINTRIN_H_INCLUDED
#define _EMMINTRIN_H_INCLUDED

/* We need definitions from the SSE header files*/
#include <xmmintrin.h>

#ifndef __SSE2__
#pragma GCC push_options
#pragma GCC target("sse2")
#define __DISABLE_SSE2__
#endif /* __SSE2__ */

stanhu · 2021-09-09T19:53:25Z

clang doesn't have these guards (https://github.com/llvm-mirror/clang/blob/master/lib/Headers/wmmintrin.h). I wonder how these CI builds are working without the options.

Should abeseil be including immintrin.h instead? https://github.com/llvm-mirror/clang/blob/aa231e4be75ac4759c236b755c57876f76e3cf05/lib/Headers/immintrin.h#L38-L40

https://stackoverflow.com/a/42493893 suggests that is the best practice.

immintrin.h is the de-factor standard header for clang and GCC to include Intel intrinsics. Using this header avoids requiring the compiler to use the `-maes` and `-msse4.1` compiler options on systems that may not have AES or SSE instruction support. clang: As seen in https://github.com/llvm-mirror/clang/blob/master/lib/Headers/immintrin.h, specific intrinsic header files are conditionally included depending on whether the feature is available. gcc: As seen in https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/immintrin.h, gcc includes all intrinsic header files, but each individual file guards against the feature not being available. This came out of an investigation in grpc/grpc#27121.

stanhu · 2021-09-09T20:05:31Z

For completeness, I submitted abseil/abseil-cpp#1015.

veblush · 2021-09-10T18:56:56Z

grpc_build_artifacts_multiplatform: running

veblush · 2021-09-10T18:57:46Z

@stanhu thanks you for investigation on this. I'm okay with removing -msse4 if this can solve the build issue.

stanhu · 2021-09-10T19:00:11Z

@veblush Yeah, I think dropping -msse4 will work for clang. Let me do that then.

This reverts commit 3b7cc74.

Older CPUs that do not have SSE4.1 would crash with the Ruby native gem due to an illegal instruction exception. The Abseil random library isn't being used at the moment (grpc#26476), and there's no reason gRPC needs to force SSE4.1 instructions on all platforms at the moment. There are other hardware-specific issues that need to be ironed out for this to work: grpc#26479 When the `-msse4` compiler flag was enabled, the Abseil code started using the `pinsrb` instruction: ``` $ elfx86exts abseil-cpp/absl/time/internal/cctz/src/time_zone_libc.o MODE64 (ret) CMOV (cmovne) SSE2 (movdqa) SSE41 (pinsrb) SSE1 (movaps) CPU Generation: Penryn ``` This was previously needed because gcc 4.8 wouldn't compile without the `-msse4` and `-maes` flags. However, per gcc-mirror/gcc@97db2bf gcc 5.0+ automatically detects whether these options are enabled. clang still needs `-maes` since including `wmmintrin.h` expects the AES option to be enabled. Closes grpc#27095

immintrin.h is the de-factor standard header for clang and GCC to include Intel intrinsics. Using this header avoids requiring the compiler to use the `-maes` and `-msse4.1` compiler options on systems that may not have AES or SSE instruction support. clang: As seen in https://github.com/llvm-mirror/clang/blob/master/lib/Headers/immintrin.h, specific intrinsic header files are conditionally included depending on whether the feature is available. gcc: As seen in https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/immintrin.h, gcc includes all intrinsic header files, but each individual file guards against the feature not being available. This came out of an investigation in grpc/grpc#27121.

veblush · 2021-09-14T23:41:09Z

Thank you for the update. Let me trigger the CI tests.

veblush · 2021-09-14T23:41:23Z

grpc_build_artifacts_multiplatform: passed

stanhu · 2021-09-17T16:02:30Z

@veblush Are these build failures relevant?

veblush

LGTM. Thank you for the PR!

veblush · 2021-09-22T23:01:13Z

@jtattermusch One more LGTM, please.

jtattermusch

LGTM once the tests are green.

Note that the test failures on MacOS look suspicious - @veblush can you please double check that they are 100% not caused by this PR before merging.

stanhu · 2021-10-04T21:08:55Z

@veblush Could you look this again?

This is even more urgent now that older versions of gRPC ship a stale cacerts that doesn't work with the recent LetsEncrypt root certificate expiration: https://gitlab.com/gitlab-org/gitlab/-/issues/342346

jtattermusch · 2021-10-05T10:34:46Z

The objC failure https://source.cloud.google.com/results/invocations/433116be-2252-4cf5-b810-9f1573fdf51f/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-test-interoptests/tests looks unrelated and all the other tests are green.
I'm going to merge.

immintrin.h is the de-factor standard header for clang and GCC to include Intel intrinsics. Using this header avoids requiring the compiler to use the `-maes` and `-msse4.1` compiler options on systems that may not have AES or SSE instruction support. clang: As seen in https://github.com/llvm-mirror/clang/blob/master/lib/Headers/immintrin.h, specific intrinsic header files are conditionally included depending on whether the feature is available. gcc: As seen in https://github.com/gcc-mirror/gcc/blob/master/gcc/config/i386/immintrin.h, gcc includes all intrinsic header files, but each individual file guards against the feature not being available. This came out of an investigation in grpc/grpc#27121.

stanhu force-pushed the sh-drop-abseil-hwflags branch from ed5335d to fd36a5d Compare August 25, 2021 07:39

stanhu force-pushed the sh-drop-abseil-hwflags branch from fd36a5d to e380b65 Compare August 25, 2021 07:41

stanhu force-pushed the sh-drop-abseil-hwflags branch 2 times, most recently from 70038f9 to b9995a7 Compare August 25, 2021 07:52

stanhu force-pushed the sh-drop-abseil-hwflags branch from b9995a7 to 3b7cc74 Compare August 25, 2021 08:09

markdroth assigned veblush Aug 25, 2021

veblush added the release notes: no Indicates if PR should not be in release notes label Sep 8, 2021

jtattermusch added the kokoro:run label Sep 9, 2021

jtattermusch requested a review from veblush September 9, 2021 11:10

grpc-kokoro removed the kokoro:run label Sep 9, 2021

stanhu mentioned this pull request Sep 9, 2021

Include immintrin.h instead of wmmintrin.h abseil/abseil-cpp#1015

Merged

stanhu added 2 commits September 10, 2021 12:00

Revert "Drop ABSL_RANDOM_HWAES_FLAGS compiler flags"

8b73897

This reverts commit 3b7cc74.

stanhu changed the title ~~Drop ABSL_RANDOM_HWAES_FLAGS compiler flags~~ Drop -msse4 compiler flag Sep 10, 2021

veblush added kokoro:run lang/core area/build labels Sep 14, 2021

grpc-kokoro removed the kokoro:run label Sep 14, 2021

veblush requested a review from jtattermusch September 22, 2021 23:00

veblush approved these changes Sep 22, 2021

View reviewed changes

jtattermusch approved these changes Sep 23, 2021

View reviewed changes

jtattermusch merged commit 106dd2f into grpc:master Oct 5, 2021

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop -msse4 compiler flag #27121

Drop -msse4 compiler flag #27121

stanhu commented Aug 25, 2021 •

edited

mttkay commented Sep 1, 2021

stanhu commented Sep 7, 2021

veblush commented Sep 8, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021 •

edited

stanhu commented Sep 9, 2021

veblush commented Sep 10, 2021

veblush commented Sep 10, 2021

stanhu commented Sep 10, 2021

veblush commented Sep 14, 2021

veblush commented Sep 14, 2021 •

edited

stanhu commented Sep 17, 2021

veblush left a comment

veblush commented Sep 22, 2021

jtattermusch left a comment

stanhu commented Oct 4, 2021

jtattermusch commented Oct 5, 2021

Drop -msse4 compiler flag #27121

Drop -msse4 compiler flag #27121

Conversation

stanhu commented Aug 25, 2021 • edited

mttkay commented Sep 1, 2021

stanhu commented Sep 7, 2021

veblush commented Sep 8, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021

stanhu commented Sep 9, 2021 • edited

stanhu commented Sep 9, 2021

veblush commented Sep 10, 2021

veblush commented Sep 10, 2021

stanhu commented Sep 10, 2021

veblush commented Sep 14, 2021

veblush commented Sep 14, 2021 • edited

stanhu commented Sep 17, 2021

veblush left a comment

Choose a reason for hiding this comment

veblush commented Sep 22, 2021

jtattermusch left a comment

Choose a reason for hiding this comment

stanhu commented Oct 4, 2021

jtattermusch commented Oct 5, 2021

stanhu commented Aug 25, 2021 •

edited

stanhu commented Sep 9, 2021 •

edited

veblush commented Sep 14, 2021 •

edited