Bug 77456 - Suboptimal code when returning a string generated with a constexpr fn at compile time
Summary: Suboptimal code when returning a string generated with a constexpr fn at comp...
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on:
Blocks: constexpr
  Show dependency treegraph
 
Reported: 2016-09-02 14:19 UTC by petschy
Modified: 2021-12-01 23:55 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
C++ source (685 bytes, text/plain)
2016-09-02 14:19 UTC, petschy
Details

Note You need to log in before you can comment on or make changes to this bug.
Description petschy 2016-09-02 14:19:36 UTC
Created attachment 39541 [details]
C++ source

I ran into this when converting expression trees to strings at compile time. Though it's surely a rare application, the fix might have positive impact on a wider range of scenarios.

The attached code converts the integers [0..N] to a string at compile time. There are several conversions with differing N's. Also, some conversions calculate the exact size of the resulting strings, others just use a large enough buffer.

Platform is is Debian Jessie, x86-64. Tested w/ 6.x and 7.0. To compile:
g++ -std=c++14 -Wall -Wextra -O3 20160831-constexpr.cpp

Please be patient, this takes almost 30 secs on my machine (AMD FX 8150 @ 4GHz), due to lots of compile-time constexpr work.

foo(): [0..13] w/ a 200 byte buffer. It seems that the initial zero fill of the buffer is not considered in dead-store elimination, so the 200 bytes are rep stos'd, then the actual characters are copied via xmm0 and bytewise literal stores:

Dump of assembler code for function _Z3foov:
   0x0000000000400620 <+0>:     mov    %rdi,%rdx
   0x0000000000400623 <+3>:     movq   $0x0,0xc0(%rdi)
   0x000000000040062e <+14>:    lea    0x8(%rdi),%rdi
   0x0000000000400632 <+18>:    mov    %rdx,%rcx
   0x0000000000400635 <+21>:    movdqa 0x27033(%rip),%xmm0        # 0x427670
   0x000000000040063d <+29>:    and    $0xfffffffffffffff8,%rdi
   0x0000000000400641 <+33>:    xor    %eax,%eax
   0x0000000000400643 <+35>:    sub    %rdi,%rcx
   0x0000000000400646 <+38>:    add    $0xc8,%ecx
   0x000000000040064c <+44>:    shr    $0x3,%ecx
   0x000000000040064f <+47>:    rep stos %rax,%es:(%rdi)
   0x0000000000400652 <+50>:    movups %xmm0,(%rdx)
   0x0000000000400655 <+53>:    movb   $0x38,0x10(%rdx)
   0x0000000000400659 <+57>:    movb   $0x20,0x11(%rdx)
   0x000000000040065d <+61>:    mov    %rdx,%rax
   0x0000000000400660 <+64>:    movb   $0x39,0x12(%rdx)
   0x0000000000400664 <+68>:    movb   $0x20,0x13(%rdx)
   0x0000000000400668 <+72>:    movb   $0x31,0x14(%rdx)
   0x000000000040066c <+76>:    movb   $0x30,0x15(%rdx)
   0x0000000000400670 <+80>:    movb   $0x20,0x16(%rdx)
   0x0000000000400674 <+84>:    movb   $0x31,0x17(%rdx)
   0x0000000000400678 <+88>:    movb   $0x31,0x18(%rdx)
   0x000000000040067c <+92>:    movb   $0x20,0x19(%rdx)
   0x0000000000400680 <+96>:    movb   $0x31,0x1a(%rdx)
   0x0000000000400684 <+100>:   movb   $0x32,0x1b(%rdx)
   0x0000000000400688 <+104>:   movb   $0x20,0x1c(%rdx)
   0x000000000040068c <+108>:   movb   $0x31,0x1d(%rdx)
   0x0000000000400690 <+112>:   movb   $0x33,0x1e(%rdx)
   0x0000000000400694 <+116>:   retq   

Since the buffer is larger, all the movb's could have been converted to another xmm0 load+store. Though an explicit zero byte is written in the C++ code after the last digit, this is missing in the disassembly above, so there is no "movb $0x00, 0x1f(%rdx)" at the end, meaning that the compiler eliminated this store, instead of merging all the 16 byte stores into a single xmm0 operation, and skipping the first 32 bytes in the rep stos.

foo_sized() generates the same string, but first it calculates the needed size. There is no zero fill here in the asm, so it was successfully eliminated, and the characters are initialized via two xmm0 loads/stores, as expected:

Dump of assembler code for function _Z9foo_sizedv:
   0x00000000004006a0 <+0>:     movdqa 0x26fc8(%rip),%xmm0        # 0x427670
   0x00000000004006a8 <+8>:     mov    %rdi,%rax
   0x00000000004006ab <+11>:    movups %xmm0,(%rdi)
   0x00000000004006ae <+14>:    movdqa 0x26fca(%rip),%xmm0        # 0x427680
   0x00000000004006b6 <+22>:    movups %xmm0,0x10(%rdi)
   0x00000000004006ba <+26>:    retq   

bar/bar_sized/bar_static/bar_sized_static(): the same as foo, but the range is [0..42], and the static versions use a static constexpr, and return the buffer pointer, not the buffer by value. 

bar() zero fills and then copies over with xmm0 and byte literals. bar_sized() lacks the zero fill, but initializes the characters the same way. The static versions just return a pointer as expected.

baz_sized() works as expected: since the memory to copy is large, it calls memcpy instead of doing the above xmm0 + literal bytes stuff.

The problem is with baz(). The range is much larger, [0..4200]. There is no DSE here either, so the buffer is first zeroed with memset, but then ALL the characters are initialized via bytewise literal stores, resulting in very large function, around 138K. Why didn't the logic kicked in that replaced the in-place init with memcpy? Or, at least, much of the copy could have been done with xmm0, copying 16 bytes at once.

One more thing: if you disable the return in fixbuf() via setting the #if 1 to 0 at line 76, interesting things will happen:

6.2.1:
g++-6.2.1 -std=c++14 -Wall -Wextra -O3 20160831-constexpr.cpp 
‘
20160831-constexpr.cpp:83: confused by earlier errors, bailing out

In a terminal window with black bg and gray font, the single quote is gray, then the error message on the next line is bold white, and it stays so, so anything I type after this will be bold white.

7.0.0: an earlier version did the same, but two days ago I built a fresh version and now it crashes:
g++-7.0.0 -std=c++14 -Wall -Wextra -O3 20160831-constexpr.cpp 
‘
In function ‘auto foo()’:
Segmentation fault
  constexpr auto x = fixbuf<13, 200>();
                                     ^
Please submit a full bug report,

The exact gcc versions used:

$ g++-6.2.1 -v
Using built-in specs.
COLLECT_GCC=g++-6.2.1
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-pc-linux-gnu/6.2.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../configure --enable-languages=c,c++ --disable-multilib --program-suffix=-6.2.1 --disable-bootstrap CFLAGS='-O2 -march=native' CXXFLAGS='-O2 -march=native'
Thread model: posix
gcc version 6.2.1 20160831 (GCC) 
git b823cdd4ccc1499a674e3863ce875c7459207727

g++-7.0.0 -v 
Using built-in specs.
COLLECT_GCC=g++-7.0.0
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/x86_64-pc-linux-gnu/7.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../configure --enable-languages=c,c++ --disable-multilib --program-suffix=-7.0.0 --disable-bootstrap CFLAGS='-O2 -march=native' CXXFLAGS='-O2 -march=native'
Thread model: posix
gcc version 7.0.0 20160831 (experimental) (GCC)
git 14c36b15d931bf299bbc214707b903d0af124449
Comment 1 Drea Pinski 2016-09-03 18:13:08 UTC
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00201.html

*** This bug has been marked as a duplicate of bug 22141 ***
Comment 2 petschy 2016-09-03 20:39:11 UTC
#22141 does not mention a DSE issue, nor a segfault of the compiler, so hardly an exact duplicate.
Comment 3 Drea Pinski 2016-09-03 20:42:23 UTC
.
Comment 4 Drea Pinski 2016-09-03 20:43:03 UTC
You have like three different bugs in listed in here so it was hard to understand which one was which :)
Comment 5 petschy 2016-09-03 20:53:21 UTC
Sorry. Should I open dedicated bugs for them, or can you work from this single one? Though the example code would be the same. Probably I would have picked a more descriptive title mentioning the DSE issue, the bogus error message formatting and the segfault.
Comment 6 Richard Biener 2016-09-05 08:07:22 UTC
Separate bugs really help.  Leaving this for the missed optimization.
Comment 7 ktkachov 2016-09-05 08:11:34 UTC
I can confirm that for the missed store merging optimisation it is fixed by my WIP patch that I'm iterating on [1].

The consecutive constant byte stores are merged into 64-bit stores.
On aarch64 the resulting assembly file at -O2 is reduced from 40757 lines to 32965 lines.

[1] https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01512.html
Comment 8 petschy 2016-09-05 12:30:17 UTC
I created two other bugs (bug 77482 for the segfault and bug 77485 for the DSE issue). As I noted in the latter, I'm a bit confused about the store merging, and what change Kyrill's patch will make, as the version compiled with gcc 7.0 somewhat merges the stores using xmm0, so the problem is not that no merging occurs, but it occurs inconsistently.

Furthermore, there must be a threshold at the amount of data above which the codegen should decide that it's more efficient to store the bytes in .rodata and memcpy to the destination than to store with multiple insns, even if merged.

This logic kicks in at baz_sized(), but does not in baz(). Interestingly, in the latter no xmm0 was used, every single byte is movb'd after the memset, whereas foo() and bar() with smaller data used xmm0, too.

Dump of assembler code for function baz():
   0x0000000000400800 <+0>:     sub    $0x8,%rsp
   0x0000000000400804 <+4>:     mov    $0x4e20,%edx
   0x0000000000400809 <+9>:     xor    %esi,%esi
   0x000000000040080b <+11>:    callq  0x4004f0 <memset@plt>
   0x0000000000400810 <+16>:    movb   $0x30,(%rax)
   0x0000000000400813 <+19>:    movb   $0x20,0x1(%rax)
   0x0000000000400817 <+23>:    movb   $0x31,0x2(%rax)
   0x000000000040081b <+27>:    movb   $0x20,0x3(%rax)
   0x000000000040081f <+31>:    movb   $0x32,0x4(%rax)
....
   0x0000000000422674 <+138868>:        movb   $0x32,0x4db3(%rax)
   0x000000000042267b <+138875>:        movb   $0x30,0x4db4(%rax)
   0x0000000000422682 <+138882>:        movb   $0x30,0x4db5(%rax)
   0x0000000000422689 <+138889>:        add    $0x8,%rsp
   0x000000000042268d <+138893>:        retq   

Even if the byte stores were merged into 64bit stores, the function still would be huge, and a memcpy instead would be way better.
Comment 9 Eric Gallager 2018-07-04 04:22:29 UTC
(In reply to petschy from comment #2)
> #22141 does not mention a DSE issue, nor a segfault of the compiler, so
> hardly an exact duplicate.

Even if it's not a duplicate, it's still related enough to go under "See Also"