Fix SSE4.1-related issues #28893

hebasto commented at 2:42 PM on November 16, 2023: member

Fix the test for SSE4.1 intrinsics during build system configuration, which currently can be false positive, for example, when CXXFLAGS="-mno-sse4.1" provided.

This PR fixes the test by adding the _mm_blend_epi16 SSE4.1 function used in our codebase.
Guard sha_x86_shani.cpp code with ENABLE_SSE41 macro as it uses the _mm_blend_epi16 function from the SSE4.1 instruction set.

It is possible that SHA-NI is enabled even when SSE4.1 is disabled, which causes compile errors in the master branch.

Closes #28864.

DrahtBot commented at 2:42 PM on November 16, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	willcl-ark, sipa, theuni
Concept NACK	luke-jr
Concept ACK	laanwj, dergoegge, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

hebasto marked this as a draft on Nov 16, 2023

hebasto force-pushed on Nov 16, 2023

DrahtBot added the label CI failed on Nov 16, 2023

laanwj commented at 3:13 PM on November 16, 2023: member

Concept ACK

dergoegge commented at 3:34 PM on November 16, 2023: member

Cool! Concept ACK

hebasto marked this as ready for review on Nov 16, 2023

maflcko added the label DrahtBot Guix build requested on Nov 16, 2023

DrahtBot removed the label CI failed on Nov 16, 2023

DrahtBot commented at 1:44 AM on November 17, 2023: contributor

Guix builds (on x86_64)

File	commit 22025d06e53f9bfccf88c600815f1cc496595d5d<br>(master)	commit 5096570b243b442ecf88f1bbb93eb483664b983a<br>(master and this pull)
SHA256SUMS.part	`8c20bf13882367c1...`	`75d5f5f74ed8cf3d...`
*-aarch64-linux-gnu-debug.tar.gz	`ddad48413a90edcc...`	`1d0fda9033fd33eb...`
*-aarch64-linux-gnu.tar.gz	`475776d445d01302...`	`dd4f058043965533...`
*-arm-linux-gnueabihf-debug.tar.gz	`4ebfac8dcd043f01...`	`35aac4c502a6def9...`
*-arm-linux-gnueabihf.tar.gz	`e99c3890d894e61d...`	`c9ed0a67ccdc2842...`
*-arm64-apple-darwin-unsigned.tar.gz	`790282cee337f0d5...`	`a34a41326f91a77c...`
*-arm64-apple-darwin-unsigned.zip	`1022a0e01d3bb7cc...`	`10593711628b82c2...`
*-arm64-apple-darwin.tar.gz	`d03565cc6bb0d273...`	`20a1dbae3d6dfbab...`
*-powerpc64-linux-gnu-debug.tar.gz	`1e6f0a493dfb280a...`	`cda5ba2e6ae593c8...`
*-powerpc64-linux-gnu.tar.gz	`d411de4850aada0f...`	`3bec634f422ccc68...`
*-powerpc64le-linux-gnu-debug.tar.gz	`86e978abca77e790...`	`437896bcfbb37d76...`
*-powerpc64le-linux-gnu.tar.gz	`ec0c9d2fc7a165c7...`	`a5fa185c67a87a7c...`
*-riscv64-linux-gnu-debug.tar.gz	`35a02a18e2a885d8...`	`34429527e4ea014a...`
*-riscv64-linux-gnu.tar.gz	`216b9b5803674e54...`	`55d89463d4ac9d4e...`
*-x86_64-apple-darwin-unsigned.tar.gz	`a4263e45b80de4ad...`	`2e1abdf0df04de80...`
*-x86_64-apple-darwin-unsigned.zip	`9c118c69289967e0...`	`080ac453313ecea2...`
*-x86_64-apple-darwin.tar.gz	`83388f8563d8043d...`	`d205f8d5a5d7b34f...`
*-x86_64-linux-gnu-debug.tar.gz	`501990251709cbba...`	`3363860af25f86b0...`
*-x86_64-linux-gnu.tar.gz	`4548a959a4d1b049...`	`8f0d5f22d8df5251...`
*.tar.gz	`ee37119e069929f2...`	`b3f67d08d8805d0c...`
guix_build.log	`a29e3e2cbad945f6...`	`f293123a8da4e497...`
guix_build.log.diff		`05d62414aebc3558...`

DrahtBot removed the label DrahtBot Guix build requested on Nov 17, 2023

fanquake commented at 5:54 PM on November 23, 2023: member

Concept ACK

luke-jr commented at 1:12 AM on December 5, 2023: member

Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it. The flag should only affect the rest of the codebase, so that the software will run on a system without SSE4.1 (but still use the optimised crypto on systems that have it).

hebasto commented at 11:09 AM on December 5, 2023: member

@luke-jr

Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it.

This PR branch still builds the SSE4.1 code if the compiler supports it, no?

DrahtBot added the label CI failed on Jan 16, 2024

DrahtBot added the label Needs rebase on Jan 27, 2024

hebasto force-pushed on Feb 12, 2024

hebasto commented at 2:27 PM on February 12, 2024: member

Rebased.

DrahtBot removed the label Needs rebase on Feb 12, 2024

DrahtBot removed the label CI failed on Feb 12, 2024

fanquake commented at 11:17 AM on March 5, 2024: member

I think this could be rebased again, given the recent build system changes, and removal of the sse4 lib.

build: Fix test for SSE4.1 intrinsics

This change uses the `_mm_blend_epi16` SSE4.1 function used in our code
and fixes false-positive cases, for example, when CXXFLAGS="-mno-sse4.1"
provided.

6ec1ca7c85

crypto: Guard code with `ENABLE_SSE41` macro

The code in `sha_x86_shani.cpp` uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set. However, it is possible that SHA-NI is
enabled even when SSE4.1 is disabled.

This changes avoid compilation errors in such a condition.

d440f13db0

hebasto force-pushed on Mar 5, 2024

hebasto commented at 11:44 AM on March 5, 2024: member

I think this could be rebased again, given the recent build system changes, and removal of the sse4 lib.

Sure! Rebased.

fanquake commented at 4:19 PM on March 5, 2024: member

@theuni probably interested here, given you might als be refactoring the define usage?

maflcko added the label DrahtBot Guix build requested on May 26, 2024

DrahtBot commented at 6:18 PM on May 26, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 327f08bb0cd91a22249395adeb34549e3c86ca76<br>(master)	commit a1b91a1424b6e5e52e62dab7a62972e2cc2a8f1d<br>(master and this pull)
SHA256SUMS.part	`2f7fbee107d854b9...`	`3634f9c71692d158...`
*-aarch64-linux-gnu-debug.tar.gz	`131ec1047dafc740...`	`c0056ca782052d2d...`
*-aarch64-linux-gnu.tar.gz	`45fb78c1e31b46db...`	`18e3ac27aec42545...`
*-arm-linux-gnueabihf-debug.tar.gz	`b6a522d6bc56f6a5...`	`cc32b6549679f1cc...`
*-arm-linux-gnueabihf.tar.gz	`decf947b99562eb5...`	`1722b603eae9da92...`
*-arm64-apple-darwin-unsigned.tar.gz	`0fff0b5c9d6d1652...`	`f880e9d57b55c98d...`
*-arm64-apple-darwin-unsigned.zip	`6db0130c624a7ddf...`	`ae18c652fbb4dfb5...`
*-arm64-apple-darwin.tar.gz	`84414b88744adffb...`	`8e4b981640a07436...`
*-powerpc64-linux-gnu-debug.tar.gz	`cb76d396c6e08279...`	`614754e0d329121a...`
*-powerpc64-linux-gnu.tar.gz	`aec752522feaaf87...`	`9641282279056855...`
*-riscv64-linux-gnu-debug.tar.gz	`b2ea26b0b0db7815...`	`ade38d756d20f814...`
*-riscv64-linux-gnu.tar.gz	`08bc325b2e2c3aeb...`	`dcf02be96c063e78...`
*-x86_64-apple-darwin-unsigned.tar.gz	`1d437b14b7dd1096...`	`be71af18e9165fe4...`
*-x86_64-apple-darwin-unsigned.zip	`f18c51926fc0efee...`	`9286a16458d230d4...`
*-x86_64-apple-darwin.tar.gz	`0833d69d18e97013...`	`823c4d9a0d195273...`
*-x86_64-linux-gnu-debug.tar.gz	`2d70f334c41d4e60...`	`0a5b4dd1436b2e85...`
*-x86_64-linux-gnu.tar.gz	`e76f8be29de9634c...`	`45bac980bbfe8266...`
*.tar.gz	`5467b517b7ad2c46...`	`72ee1a8832d6146e...`
guix_build.log	`d68b4ff13be02f84...`	`6f82440b85f6e39c...`
guix_build.log.diff		`92a1b24a0c948c2c...`

DrahtBot removed the label DrahtBot Guix build requested on May 26, 2024

willcl-ark commented at 6:22 PM on July 1, 2024: member

Concept ACK.

I was tacking this today (missed this PR as GH search for "sse" yielded no results 🤦🏼‍♂️ )

First I tried re-implementing @luke-jr previous approach, but refactoring into macros: https://github.com/bitcoin/bitcoin/commit/cdaa9bfd6e63dc675cdd27e6d586f69423ee1748

Whilst this works well-enough, it didn't particularly fix #28864 in my opinion, so I tried https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:detect-sse4.1

Now that I've found this PR, the mechanics of the two change-sets appear very similar, but @hebasto gates SHANI from the Makefile whereas I opted to gate in configure.ac. Not sure if either has any advantages, but I'm concept ACK here!

willcl-ark approved

willcl-ark commented at 9:58 AM on July 2, 2024: member

tACK d440f13db02c82c842000abe4fe4d0c721a4ad3b

These changes fix the issues correctly in my testing on Ubuntu.

I do slightly prefer how my changeset printed a configure-time warning message for the user that SHANI support would be disabled if SSE4.1 was not enabled, but overall prefer the approach of this fix.

DrahtBot requested review from laanwj on Jul 2, 2024

DrahtBot requested review from dergoegge on Jul 2, 2024

DrahtBot requested review from fanquake on Jul 2, 2024

maflcko added the label Build system on Jul 2, 2024

maflcko added the label Needs CMake port on Jul 2, 2024

DrahtBot added the label CI failed on Jul 14, 2024

DrahtBot removed the label CI failed on Jul 16, 2024

sipa commented at 3:51 PM on July 16, 2024: member

utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b

theuni approved

theuni commented at 3:48 PM on July 17, 2024: member

utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b

fanquake merged this on Jul 17, 2024

fanquake closed this on Jul 17, 2024

hebasto deleted the branch on Jul 17, 2024

hebasto commented at 5:44 PM on July 18, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/268.

hebasto removed the label Needs CMake port on Jul 18, 2024

hebasto referenced this in commit 9b4aa929b0 on Jul 18, 2024

kwvg referenced this in commit 643785c121 on Jul 12, 2025

kwvg referenced this in commit e7cc75af55 on Jul 15, 2025

kwvg referenced this in commit f4e3b0eab8 on Jul 15, 2025

kwvg referenced this in commit 5f03ab46fa on Jul 15, 2025

kwvg referenced this in commit dbe8bb04a1 on Jul 15, 2025

kwvg referenced this in commit 76cef17bd0 on Jul 16, 2025

PastaPastaPasta referenced this in commit d5fc8bf710 on Jul 17, 2025

bitcoin locked this on Jul 18, 2025