OpenSSL perf regression trunk vs. Clang 9

[davidbolvansky]

Bugzilla Link	44409
Resolution	INVALID
Resolved on	Jan 16, 2020 03:33
Version	trunk
OS	Linux
Blocks	#43756
CC	@adibiagio,@topperc,@dwblaikie,@fhahn,@LebedevRI,@RKSimon,@rotateright,@yuanfang-chen

Extended Description

As mentioned in
https://www.phoronix.com/scan.php?page=article&item=gcc-clang-3960x&num=6

28 % worse.

[fhahn]

Interesting! Do you know if there are instructions on how to best reproduce the regressions? It seems they are using https://github.com/phoronix-test-suite/phoronix-test-suite/

[RKSimon]

The PTS entry for OpenSSL:

https://openbenchmarking.org/test/pts/openssl-1.11.0

and the benchmark scripts:

https://openbenchmarking.org/innhold/ab4a63d1b9feb3770d411314e243f9f2b2809471

[adibiagio]

This is the actual link for the script used by Phoronix:
https://openbenchmarking.org/innhold/ac54d3a761f1918e5130fa427eed2ded19e8f8b5

I had to slightly tweak the build flags to make sure that the correct -march flag is passed to make (I am passing -march=native; on my host PC it is znver1).

Numbers are "signs per second". The bigger the better.

            GCC 7.4   |   CLANG-9   | CLANG master
          ------------+-------------+-------------

1 Thread 197.1 | 193.7 | 148.1

16 Threads 1584.7 | 1584.6 | 1206.4

I see a 24% slowdown with CLANG master.
I am going to git rebase my

--

Below is the full output:

[gcc 7.4]

(16 threads)

OpenSSL 1.1.1 11 Sep 2018
built on: Wed Jan 15 11:21:04 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: gcc -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.000631s 0.000010s 1584.7 102683.7

(1 thread)

OpenSSL 1.1.1 11 Sep 2018
built on: Wed Jan 15 11:21:04 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: gcc -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.005074s 0.000078s 197.1 12852.3

[clang 9]

(16 threads)

OpenSSL 1.1.1 11 Sep 2018
built on: Tue Jan 14 19:57:09 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: clang -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.000631s 0.000010s 1584.6 102513.9

(1 thread)

OpenSSL 1.1.1 11 Sep 2018
built on: Tue Jan 14 19:57:09 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: clang -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.005163s 0.000079s 193.7 12660.3

[clang master]

(16 threads)

OpenSSL 1.1.1 11 Sep 2018
built on: Tue Jan 14 19:49:28 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: clang -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.000829s 0.000013s 1206.4 78357.3

(1 thread)

OpenSSL 1.1.1 11 Sep 2018
built on: Tue Jan 14 19:49:28 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: clang -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.006752s 0.000103s 148.1 9662.5

[adibiagio]

tl;dr this is NOT a compiler bug.
An incorrect regexpr is used by the perl script crypto/bn/asm/x86_64-mont5.pl to check the clang version. The version for clang-10 contains two digits before the first dot. However, the regexpr used in that script expects only a single digit!.

If that regexpr fails to match, then a less optimized hand-written assembly function is used to perform what looks like two parallel chains add-with-carry and multiplies (what seems to be a large integer multiplication).
If however there is a match in the regexpr, and the clang version is > 3.03, then an different variant of that same function is used on targets that feature ADX.

That optimized variant uses intel ADX instructions such as MULX/ADCX/ADOX.

For the record: this is the result from perf on my znver1

perf record -s --no-inherit -F99 ./apps/openssl speed rsa4096
perf report

[ Clang 9 ]
Samples: 1K of event 'cycles', Event count (approx.): 77208766013

47.53% openssl libcrypto.so.1.1 [.] __bn_sqrx8x_reduction
25.62% openssl libcrypto.so.1.1 [.] bn_sqrx8x_internal
11.42% openssl libcrypto.so.1.1 [.] mulx4x_internal
9.56% openssl libcrypto.so.1.1 [.] bn_mulx4x_mont
2.35% openssl libcrypto.so.1.1 [.] bn_mul_add_words

[ CLANG master ]

Samples: 1K of event 'cycles', Event count (approx.): 76240135925

Hottest functions by sample:

45.88% openssl libcrypto.so.1.1 [.] __bn_sqr8x_reduction
26.96% openssl libcrypto.so.1.1 [.] bn_sqr8x_internal
11.06% openssl libcrypto.so.1.1 [.] mul4x_internal
10.86% openssl libcrypto.so.1.1 [.] bn_mul4x_mont
2.16% openssl libcrypto.so.1.1 [.] bn_mul_add_words

NOTE how the hottest symbol is not the same (I didn't catch this the first time I saw it! duh...) . In Clang master it is __bn_sqr8x_reduction. In Clang 9 it is __bn_sqrx8x_reduction (notice the extra x).

Hottest basic block in __bn_sqrx8x_reduction

Clang 9 codegen with sample percentages:

   │189:   mov    %r8,%rbx
   │       mulx   0x0(%rbp),%rax,%r8

0.11 │ adcx %rax,%rbx
3.97 │ adox %r9,%r8
3.64 │ mulx 0x8(%rbp),%rax,%r9
0.43 │ adcx %rax,%r8
4.29 │ adox %r10,%r9
6.22 │ mulx 0x10(%rbp),%rax,%r10
│ adcx %rax,%r9
0.96 │ adox %r11,%r10
5.79 │ mulx 0x18(%rbp),%rax,%r11
0.75 │ adcx %rax,%r10
0.43 │ adox %r12,%r11
9.22 │ mulx 0x20(%rbp),%rax,%r12
0.75 │ adcx %rax,%r11
3.86 │ adox %r13,%r12
4.50 │ mulx 0x28(%rbp),%rax,%r13
0.11 │ adcx %rax,%r12
0.11 │ adox %r14,%r13
10.61 │ mulx 0x30(%rbp),%rax,%r14
│ adcx %rax,%r13
0.32 │ adox %r15,%r14
8.47 │ mulx 0x38(%rbp),%rax,%r15
│ mov 0x80(%rsp,%rcx,8),%rdx
│ adcx %rax,%r14
2.04 │ adox %rsi,%r15
5.04 │ mov %rbx,(%rdi,%rcx,8)
│ mov %r8,%rbx
│ adcx %rsi,%r15
5.47 │ inc %rcx
│ ↑ jne 189

Hottest basic block in __bn_sqr8x_reduction

Clang 9 codegen with sample percentages:
0.11 │187: mul %rbx
0.88 │ add %rax,%r8
1.54 │ mov 0x8(%rbp),%rax
0.11 │ mov %r8,(%rdi)
2.20 │ mov %rdx,%r8
1.21 │ adc $0x0,%r8
1.32 │ mul %rbx
0.99 │ add %rax,%r9
1.54 │ mov 0x10(%rbp),%rax
0.22 │ adc $0x0,%rdx
3.30 │ add %r9,%r8
2.20 │ lea 0x8(%rdi),%rdi
0.44 │ mov %rdx,%r9
│ adc $0x0,%r9
2.86 │ mul %rbx
1.21 │ add %rax,%r10
1.98 │ mov 0x18(%rbp),%rax
0.11 │ adc $0x0,%rdx
3.52 │ add %r10,%r9
1.43 │ mov %rdx,%r10
│ adc $0x0,%r10
... (truncated for simplicity)

PROBLEM

All that code is hand-written and expanded by the perl script crypto/bn/asm/x86_64-mont5.pl.

A branch to the optimized variant is only expanded if variable $addx in the above mentioned perl script is set. See below how that variable is set:

if (`$ENV{CC} -Wa,-v -c -o /dev/null -x assembler /dev/null 2>&1`
                =~ /GNU assembler version ([2-9]\.[0-9]+)/) {
        $addx = ($1>=2.23);
}

if (!$addx && $win64 && ($flavour =~ /nasm/ || $ENV{ASM} =~ /nasm/) &&
            `nasm -v 2>&1` =~ /NASM version ([2-9]\.[0-9]+)/) {
        $addx = ($1>=2.10);
}

if (!$addx && $win64 && ($flavour =~ /masm/ || $ENV{ASM} =~ /ml64/) &&
            `ml64 2>&1` =~ /Version ([0-9]+)\./) {
        $addx = ($1>=12);
}

if (!$addx && `$ENV{CC} -v 2>&1` =~ /((?:^clang|LLVM) version|.*based on LLVM) ([3-9])\.([0-9]+)/) {
        my $ver = $2 + $3/100.0;        # 3.1->3.01, 3.10->3.10
        $addx = ($ver>=3.03);
}

In the case of clang-9, we have:

clang -v

clang version 9.0.1

The last if block is executed, and we set $add. That is because $2 is 9, and $3 is 0.
expression ($ver>=3.03) evaluates to true, and $addx is therefore set.

--
with clang 10 we have:

clang -v

clang version 10.0.0

The last regexpr only expects a single digit before the first dot!:

((?:^clang|LLVM) version|.*based on LLVM) ([3-9])\.([0-9]+)/)

So, we don't match the first digit, and we don't set $addx.

Later on we have this block of perl:

$code.=<<___ if ($addx);
        mov     OPENSSL_ia32cap_P+8(%rip),%r11d
        and     \$0x80108,%r11d
        cmp     \$0x80108,%r11d         # check for AD*X+BMI2+BMI1
        jne     .Lfrom_mont_nox

        lea     (%rax,$num),$rptr
        call    __bn_sqrx8x_reduction
        call    __bn_postx4x_internal

        pxor    %xmm0,%xmm0
        lea     48(%rsp),%rax
        jmp     .Lfrom_mont_zero

.align  32
.Lfrom_mont_nox:
___
$code.=<<___;
        call    __bn_sqr8x_reduction
        call    __bn_post4x_internal

        pxor    %xmm0,%xmm0
        lea     48(%rsp),%rax
        jmp     .Lfrom_mont_zero

Basically the call to __bn_sqrx8x_reduction is only expanded if $addx was set!.

====

As an experiment, I have changed the regexpr used by that last if statement in perl to this:

if (!$addx && `$ENV{CC} -v 2>&1` =~ /((?:^clang|LLVM) version|.*based on LLVM) ([0-9]+)\.([0-9]+)/) {
        my $ver = $2 + $3/100.0;        # 3.1->3.01, 3.10->3.10
        $addx = ($ver>=3.03);
}

This is "enough" to recover the full perf loss (see below the results for the single threaded speed run of rsa4096)

OpenSSL 1.1.1 11 Sep 2018
built on: Wed Jan 15 16:21:33 2020 UTC
options:bn(64,64) rc4(8x,int) des(int) aes(partial) idea(int) blowfish(ptr)
compiler: clang -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -march=native -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOP
ENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DKECCAK1600_ASM -DRC4_ASM -DMD5
_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DGHASH_ASM -DECP_NISTZ256_ASM -DX25519_ASM -DPADLOCK_ASM -DPOLY1305_ASM -DNDEBUG
sign verify sign/s verify/s
rsa 4096 bits 0.005176s 0.000104s 193.2 9597.9

This bug should be reported to OpenSSL so that they can fix their script.
Thankfully this is not a compiler regression.

[fhahn]

awesome, thanks for digging into this!

[adibiagio]

Reported here: openssl/openssl#10853

[davidbolvansky]

Closed, not a codegen regression.

[yuanfang-chen]

Reported here: openssl/openssl#10853

Great analysis, Andrea!

[adibiagio]

Just for the record:
This is going to be fixed on OpenSSL master. The same fix is going to be backported to OpenSSL 1.1.1 (the one used by Phoronix)

openssl/openssl#10855
openssl/openssl#10857

It means that future runs of openssl (built by clang 10) should not show perf regressions anymore.

[davidbolvansky]

Great!

[davidbolvansky]

mentioned in issue #43756