[Bug c++/106804] New: Poor codegen for selecting and incrementing value behind a reference

[Bug c++/106804] New: Poor codegen for selecting and incrementing value behind a reference

[anthony.mikh at yandex dot ru]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106804

            Bug ID: 106804
           Summary: Poor codegen for selecting and incrementing value
                    behind a reference
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: anthony.mikh at yandex dot ru
  Target Milestone: ---

Godbolt: https://godbolt.org/z/e9ePs7Ece

For the following source code:

    void increment_largest(int& a, int& b) {
        ++(a > b ? a : b);
    }

gcc 12 with -O2 produces the following asm:

increment_largest(int&, int&):
        mov     edx, DWORD PTR [rdi]
        mov     eax, DWORD PTR [rsi]
        cmp     edx, eax
        jle     .L2
        add     edx, 1
        mov     DWORD PTR [rdi], edx
        ret
.L2:
        add     eax, 1
        mov     DWORD PTR [rsi], eax
        ret

For equivalent code using pointers:

    void increment_largest(int* a, int* b) {
        ++*(*a > *b ? a : b);
    }

gcc with -O2 gives something slightly different:

increment_largest(int*, int*):
        mov     edx, DWORD PTR [rdi]
        mov     eax, DWORD PTR [rsi]
        cmp     edx, eax
        jle     .L2
        mov     eax, edx
        mov     rsi, rdi
.L2:
        add     eax, 1
        mov     DWORD PTR [rsi], eax
        ret

If one rewrites code with references to assign the selected reference to a
variable:

    void increment_largest(int& a, int& b) {
        auto& tgt = (a > b ? a : b);
        ++tgt;
    }

it gives exactly the same asm as the version with pointers. Anyway it is
seemingly worse than what clang-14 -O2 produces for all three sources:

increment_largest(int&, int&):
        mov     eax, dword ptr [rdi]
        cmp     eax, dword ptr [rsi]
        cmovg   rsi, rdi
        add     dword ptr [rsi], 1
        ret

Likely to be related to PR94006.

[Bug middle-end/106804] Poor codegen for selecting and incrementing value behind a reference

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106804

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note this code gen is a microbenchmarking and might not make a real difference
if the function is inlined or even part of some bigger code.

[Bug middle-end/106804] Poor codegen for selecting and incrementing value behind a reference

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106804

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Clang use of cmov also might produce worse code

[Bug middle-end/106804] Poor codegen for selecting and incrementing value behind a reference

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106804

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Yeah, the sink pass sink_common_stores_to_bb could be enhanced to do this
though it's somewhat questionable when doing so is profitable - it requires
two PHIs, one for the pointer and one for the stored value, which means
possibly two copies on the edge(es) to the common store.  In fact I'd say
the reverse transformation is more profitable?

[Bug middle-end/106804] Poor codegen for selecting and incrementing value behind a reference

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106804

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org

--- Comment #8 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #7)
> In fact I'd say the reverse transformation is more profitable?

In the end it depends on the context. It's a trade-off between a conditional
branch and extra data dependencies feeding into the address of a store. If a
branch is perfectly predictable, it's preferable. Otherwise, if there's no
memory dependency via the store, you don't care about delaying it, making the
branchless version preferable if that reduces pipeline flushes. If there is a
dependency, it comes down to how often the branch mispredicts, I guess.

  ________________________________
 /                                \
| People who tinker with compilers |
| need __builtin_branchless_select |
 \                                /
  ================================
                                     \
                                      \
                                       \
                                        .--.
                                       |o_o |
                                       | ~  |
                                      //   \ \
                                     (|     | )
                                    /'\_   _/`\
                                    \___)=(___/