Add POWER8 ASM for 4-way SHA256 #13203

TheBlueMatt · 2018-05-09T17:46:35Z

Based on #13191, This speeds up 4-way SHA256 by about 3.75x over the C impl.

laanwj · 2018-05-09T18:02:09Z

luke-jr

I'd be surprised if this works as-is; either way, some cleanup needed to avoid breaking non-POWER builds.

luke-jr · 2018-05-09T18:20:48Z

configure.ac

@@ -762,6 +762,29 @@ AC_LINK_IFELSE([AC_LANG_SOURCE([
 )
 LDFLAGS="$TEMP_LDFLAGS"

+TEMP_LDFLAGS="$LDFLAGS"
+LDFLAGS="$TEMP_LDFLAGS -mpower8-vector"


Shouldn't this be CPPFLAGS or CXXFLAGS?

Agree, but this should be assigned to POWER_VSX_CXXFLAGS or so above, in an AX_CHECK_LINK_FLAG test.

luke-jr · 2018-05-09T18:21:36Z

src/Makefile.am

@@ -309,6 +310,16 @@ crypto_libbitcoin_crypto_avx2_a_CPPFLAGS += -DENABLE_AVX2
 endif
 crypto_libbitcoin_crypto_avx2_a_SOURCES = crypto/sha256_avx2.cpp

+if HAVE_POWER8
+crypto_libbitcoin_crypto_power8_a_CPPFLAGS = $(AM_CPPFLAGS) -mpower8-vector


This is probably needed on CXXFLAGS, since the man page says it changes the code generated.

luke-jr · 2018-05-09T18:27:18Z

src/crypto/sha256.cpp

@@ -29,6 +29,15 @@ namespace sha256d64_avx2
 void Transform_8way(unsigned char* out, const unsigned char* in);
 }

+#if defined(__linux__) && defined(HAVE_POWER8)
+#include <sys/auxv.h>


I don't think we need this here, and POWER vectoring stuff seems to screw with standard C++ stuff, so maybe not a good idea to have it.

auxv isnt POWER- or vector-specific, its to get getauxval which is a libc function.

luke-jr · 2018-05-09T18:28:04Z

src/crypto/sha256.cpp

@@ -511,6 +520,12 @@ std::string SHA256AutoDetect()
        ret = "sse4";
 #endif
    }
+#elif (defined(__linux__)) && defined(HAVE_POWER8)
+    if (getauxval(AT_HWCAP2) & 0x02000000) {


Can we move this check to a function in the power8-specific file?

Its right below the x86-specific support check, and its a short file, it seems like overkill.

luke-jr · 2018-05-09T18:29:04Z

src/crypto/sha256_power8.cpp

+{
+
+typedef __vector uint32_t uint32x4_p8;
+typedef __vector uint8_t uint8x16_p8;


__vector is a non-standard GCC extension; any reason we can't use vector here directly?

luke-jr · 2018-05-09T18:29:50Z

src/crypto/sha256_power8.cpp

+#include <config/bitcoin-config.h>
+#endif
+
+#ifdef HAVE_POWER8


Since this file is only compiled with HAVE_POWER8, I think we don't need this here.

theuni

Concept ACK!

First set of build-system comments, there are few things to sort out. Will continue reviewing after the first round of cleanups.

theuni · 2018-05-09T19:19:24Z

src/Makefile.am

+crypto_libbitcoin_crypto_avx2_a_CPPFLAGS = $(CPPFLAGS)
+if ENABLE_AVX2
+crypto_libbitcoin_crypto_avx2_a_CXXFLAGS += $(AVX2_CXXFLAGS)
+crypto_libbitcoin_crypto_avx2_a_CPPFLAGS += -DENABLE_AVX2


Can't compiler defines be used instead so that non-autoconf builds can compile with optims too?

Specifically for this one, rather than ifdef(ENABLE_AVX2), use ifdef(__AVX2__)

theuni · 2018-05-09T19:28:22Z

configure.ac

@@ -762,6 +762,29 @@ AC_LINK_IFELSE([AC_LANG_SOURCE([
 )
 LDFLAGS="$TEMP_LDFLAGS"

+TEMP_LDFLAGS="$LDFLAGS"
+LDFLAGS="$TEMP_LDFLAGS -mpower8-vector"


Agree, but this should be assigned to POWER_VSX_CXXFLAGS or so above, in an AX_CHECK_LINK_FLAG test.

theuni · 2018-05-09T19:31:49Z

configure.ac

@@ -327,6 +330,45 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
 )
 CXXFLAGS="$TEMP_CXXFLAGS"

+AC_DEFINE(ENABLE_AVX2, 1, [Define this symbol to build code that uses AVX2 intrinsics])


Only define these in their respective check's success cases.

theuni · 2018-05-09T19:34:41Z

configure.ac

+  }
+  ]])],
+  [
+    AC_DEFINE(HAVE_POWER8,1,[Define if compiler supports POWER8 instructions.])


Please move the defines/conditionals down with the other asm flags for consistency.

theuni · 2018-05-09T19:35:48Z

src/Makefile.am

@@ -289,6 +294,32 @@ if USE_ASM
 crypto_libbitcoin_crypto_a_SOURCES += crypto/sha256_sse4.cpp
 endif

+crypto_libbitcoin_crypto_sse41_a_CXXFLAGS = $(CXXFLAGS)


These should all be $(AM_CXXFLAGS) $(PIE_FLAGS).

theuni · 2018-05-09T19:36:06Z

src/Makefile.am

@@ -289,6 +294,32 @@ if USE_ASM
 crypto_libbitcoin_crypto_a_SOURCES += crypto/sha256_sse4.cpp
 endif

+crypto_libbitcoin_crypto_sse41_a_CXXFLAGS = $(CXXFLAGS)
+crypto_libbitcoin_crypto_sse41_a_CPPFLAGS = $(CPPFLAGS)


Ditto for the CPPFLAGS.

theuni · 2018-05-09T19:42:50Z

src/Makefile.am

+crypto_libbitcoin_crypto_power8_a_SOURCES = crypto/sha256_power8.cpp
+
+LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_POWER8)
+EXTRA_LIBRARIES += $(LIBBITCOIN_CRYPTO_POWER8)


append to EXTRA_LIBRARIES outside of the if HAVE_POWER8. These libraries are only built if something depends on them.

theuni · 2018-05-09T19:46:50Z

src/Makefile.am

+crypto_libbitcoin_crypto_power8_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+crypto_libbitcoin_crypto_power8_a_SOURCES = crypto/sha256_power8.cpp
+
+LIBBITCOIN_CRYPTO += $(LIBBITCOIN_CRYPTO_POWER8)


I'm not sure whether this works as intended or not, but we don't want to be doing it either way. Add LIBBITCOIN_CRYPTO_POWER8 to bitcoind_LDADD like the others, we can potentially aggregate them as a follow-up.

Why not? Its shorthand for "crypto depends on this" instead of listing it as a dep in every target that may use it. Its also much less britle, which is important given we dont have a POWER travis target.

I agree with aggregating, but as a clean follow-up, or as part of #13191. I'd just prefer not to have one-off's. It's not clear, for example, if EXTRA_LIBRARIES is appended twice, as it depends on when LIBBITCOIN_CRYPTO is calculated.

TheBlueMatt · 2018-05-09T20:59:18Z

@luke-jr No, it definitely works, but its apparently /only/ tested on POWER, not "not yet tested" :p.

theuni · 2018-05-09T21:57:45Z

configure.ac

+  ]
+)
+AM_CONDITIONAL([HAVE_POWER8], [$HAVE_POWER8])
+LDFLAGS="$TEMP_LDFLAGS"


This should reset CXXFLAGS, not LDFLAGS.

theuni · 2018-05-09T23:20:20Z

configure.ac

+  ]])],
+  [
+    AC_DEFINE(HAVE_POWER8,1,[Define if compiler supports POWER8 instructions.])
+    AC_MSG_RESULT(yes)


POWER8_ASM_CXXFLAGS="-mpower8-vector" AC_MSG_RESULT(yes)

Then at the bottom, with the others:

AC_SUBST(POWER8_ASM_CXXFLAGS)

Then in the Makefile.am:

crypto_libbitcoin_crypto_power8_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) $(POWER8_ASM_CXXFLAGS)

theuni · 2018-05-09T23:24:03Z

configure.ac

+    AC_MSG_RESULT(no)
+  ]
+)
+AM_CONDITIONAL([HAVE_POWER8], [$HAVE_POWER8])


HAVE_POWER8 is a define that will be handled by autoheader, it can't be treated like a variable here. Need to set one separately.

gmaxwell · 2018-05-15T18:55:37Z

spiffy! will test soon

TheBlueMatt · 2018-06-04T18:34:45Z

Rebased.

This speeds up 4-way SHA256 by about 3.75x over the C impl.

Previously checkqueue bench would essentially just test cross-core latency with more threads the more threads a host had. Not only did this make the result worse on hosts with more CPU cores, but it doesn't mimic how we use the checkqueue in real life, and made bench_bitcoin almost unusably slow on high-core-count systems, especially dual-CPU systems. Instead, we use the same thread count that we use for the real validation queue, with a 75-microsecond busy-wait on each thread, which should at least vaguely mimic how we use the checkqueue.

TheBlueMatt · 2018-06-11T16:32:40Z

Rebased. Should be ready for merge +/- some more review, though luckily the tests should mostly cover it.

TheBlueMatt · 2018-06-11T16:33:32Z

Note that I also had to fix the checkqueue bench as otherwise bench_bitcoin is unsuably slow on multi-cpu (or many-core) systems.

laanwj · 2018-06-14T17:50:48Z

I'm unable to test this at the moment. Can someone please post the benchmark results before/after this?

DrahtBot · 2018-07-09T23:26:52Z

Needs rebase

luke-jr · 2018-07-25T23:22:25Z

src/crypto/sha256_power8.cpp

+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+//
+// This is a translation to GCC extended asm syntax from YASM code by Intel
+// (available at the bottom of this file).


I don't see any YASM code at the bottom; also, really from Intel?

Looks like that was copied from the sse4 code.

luke-jr · 2018-07-25T23:52:19Z

configure.ac

+    #include <stdint.h>
+  ]], [[
+    unsigned char src[16];
+    __builtin_crypto_vshasigmaw((__vector uint32_t)vec_vsx_ld(0, src), 1, 0xf);


Better to use vector here. We don't use [the non-standard] __vector in the code itself.

luke-jr · 2018-07-25T23:54:49Z

configure.ac

@@ -314,6 +314,7 @@ fi
 AX_CHECK_COMPILE_FLAG([-msse4.2],[[SSE42_CXXFLAGS="-msse4.2"]],,[[$CXXFLAG_WERROR]])
 AX_CHECK_COMPILE_FLAG([-msse4.1],[[SSE41_CXXFLAGS="-msse4.1"]],,[[$CXXFLAG_WERROR]])
 AX_CHECK_COMPILE_FLAG([-mavx -mavx2],[[AVX2_CXXFLAGS="-mavx -mavx2"]],,[[$CXXFLAG_WERROR]])
+AX_CHECK_COMPILE_FLAG([-mpower8-vector],[[POWER8_CXXFLAGS="-mpower8-vector"]],,[[$CXXFLAG_WERROR]])


I think -faltivec is needed too?

gmaxwell · 2018-08-21T19:16:38Z

This needs a trivial rebase but with that works fine against master. ACK. Will post benchmarks after IBD finishes.

gmaxwell · 2018-08-23T02:08:43Z

Before:
MerkleRoot, 5, 800, 21.6763, 0.00541869, 0.00541984, 0.00541886
After:
MerkleRoot, 5, 800, 6.32571, 0.00158138, 0.00158148, 0.00158142

On power9.

Interesting reindex speed was about the same for both.

gmaxwell · 2018-08-23T02:09:31Z

@TheBlueMatt Rebase.

TheBlueMatt · 2018-08-24T19:31:53Z

Hmm, well if it doesnt speed up rebase then there is something funky we should look into before merging new code, no?

TheBlueMatt · 2018-08-24T20:29:52Z

Gonna go ahead and close it, if someone wants to adopt it and figure out why IBD didn't improve where the equivalent x86 change improved it significantly, they should adopt it.

gmaxwell · 2018-08-24T23:46:23Z

The comparable change made a big impact on x86 many months ago-- who knows now... Synchronization primitives are more expensive on power, so it also wouldn't be surprising if our glib use of atomics all over the place weren't moving around the limiting factors in performance on power compared to x86.

TheBlueMatt force-pushed the 2018-05-asm branch from 93deb36 to 67f6440 Compare May 9, 2018 17:47

luke-jr suggested changes May 9, 2018

View reviewed changes

laanwj added the Validation label May 9, 2018

theuni reviewed May 9, 2018

View reviewed changes

theuni mentioned this pull request May 9, 2018

Specialized double-SHA256 with 64 byte inputs with SSE4.1 and AVX2 #13191

Merged

TheBlueMatt force-pushed the 2018-05-asm branch 2 times, most recently from 184d626 to 9329896 Compare May 9, 2018 21:05

theuni reviewed May 9, 2018

View reviewed changes

TheBlueMatt force-pushed the 2018-05-asm branch 2 times, most recently from 667816c to 84c7c7d Compare May 9, 2018 22:27

theuni reviewed May 9, 2018

View reviewed changes

TheBlueMatt force-pushed the 2018-05-asm branch from 84c7c7d to 41e09f4 Compare May 10, 2018 16:04

TheBlueMatt force-pushed the 2018-05-asm branch from 41e09f4 to 6a26ce9 Compare June 4, 2018 18:34

This was referenced Jun 5, 2018

SHA256 implementations based on Intel SHA Extensions #13386

Merged

crypto: cleanup sha256 build #13408

Merged

DrahtBot added the Needs rebase label Jun 11, 2018

TheBlueMatt force-pushed the 2018-05-asm branch 3 times, most recently from 46b601d to 6174209 Compare June 11, 2018 16:26

TheBlueMatt added 2 commits June 11, 2018 12:27

Add POWER8 vector impl for 4-way SHA256

b04f117

This speeds up 4-way SHA256 by about 3.75x over the C impl.

TheBlueMatt force-pushed the 2018-05-asm branch from 6174209 to 3b402e0 Compare June 11, 2018 16:27

maflcko removed the Needs rebase label Jun 11, 2018

DrahtBot added the Needs rebase label Jul 9, 2018

luke-jr reviewed Jul 25, 2018

View reviewed changes

TheBlueMatt closed this Aug 24, 2018

luke-jr mentioned this pull request Oct 21, 2018

gitian-linux: Build binaries for 64-bit POWER #14066

Closed

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Add POWER8 ASM for 4-way SHA256 #13203

Add POWER8 ASM for 4-way SHA256 #13203

Uh oh!

Conversation

TheBlueMatt commented May 9, 2018

Uh oh!

laanwj commented May 9, 2018

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented May 15, 2018

Uh oh!

TheBlueMatt commented Jun 4, 2018

Uh oh!

TheBlueMatt commented Jun 11, 2018

Uh oh!

TheBlueMatt commented Jun 11, 2018

Uh oh!

laanwj commented Jun 14, 2018

Uh oh!

DrahtBot commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!