[Bug c++/95405] New: Unnecessary stores with std::optional

[Bug c++/95405] New: Unnecessary stores with std::optional

[steffen.hirschmann at ipvs dot uni-stuttgart.de]

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

            Bug ID: 95405
           Summary: Unnecessary stores with std::optional
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: steffen.hirschmann at ipvs dot uni-stuttgart.de
  Target Milestone: ---

I posted this to the gcc-help mailing list a few days ago
(https://gcc.gnu.org/pipermail/gcc-help/2020-May/138978.html).

GCC produces stores that don't seem to be required for std::optional.

Code:
--------
#include <optional>
std::optional<long> foo();
long bar()
{
    auto r = foo();
    if (r)
        return *r;
    else
        return 0L;
}
--------

What gcc 10.1 with -std=c++17 -O3 produces is:
bar():
        sub     rsp, 24
        call    foo()
        mov     QWORD PTR [rsp+8], rdx
        cmp     BYTE PTR [rsp+8], 0
        mov     QWORD PTR [rsp], rax
        mov     rax, QWORD PTR [rsp]
        jne     .L1
        xor     eax, eax
.L1:
        add     rsp, 24
        ret

(see: https://godbolt.org/z/uHE6QB)

I don't understand the stores (and loads) after the call to foo. They
don't seem necessary to me.


Marc Glisse pointed out
(https://gcc.gnu.org/pipermail/gcc-help/2020-May/138982.html) that the first
pair of store/load seems to be a tuning choice and can be removed with the
correct tuning flags.


What I expected is:
        mov     QWORD PTR [rsp+8], rdx
        cmp     BYTE PTR [rsp+8], 0
should be a compare/test directly of dl.

And:
        mov     QWORD PTR [rsp], rax
        mov     rax, QWORD PTR [rsp]
is not present at all.

Can someone explain this behavior? Shouldn't the optimizer produce what I
pointed out?

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-03-07
          Component|tree-optimization           |rtl-optimization
             Target|                            |x86_64-linux-gnu
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
On aarch64 we do the correct thing:
_Z3barv:
        stp     x29, x30, [sp, -32]!
        mov     x29, sp
        bl      _Z3foov
        tst     w1, 255
        csel    x0, x0, xzr, ne
        ldp     x29, x30, [sp], 32
        ret

So this is most likely an issue with how x86_64 implements stuff.

Confirmed for x86_64 though.

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[gabravier at gmail dot com]

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

Gabriel Ravier <gabravier at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gabravier at gmail dot com

--- Comment #2 from Gabriel Ravier <gabravier at gmail dot com> ---
Welp, I've tried to convert this to a simplified form, but I can't seem to get
the same output regardless of how close I get in terms of GIMPLE output.

With this code:

struct opbeb {};

union opbs {
        opbeb empty_byte;
        long value;
};

struct opb {
        opbs payload;
        bool engaged;
};

struct op : public opb {
};

struct ob {
        op payload;
};

struct o {
        ob base;
};

o foo();

long bar()
{
        struct o r = foo();
        if (__builtin_expect_with_probability((*(const ob *)&r).payload.engaged
!= 0, 1, .66))
                return (long &)*(long *)&r;
        else
                return 0;
}

I get this final GIMPLE (i.e. -fdump-tree-optimized):

;; Function bar (_Z3barv, funcdef_no=9255, decl_uid=109154, cgraph_uid=6606,
symbol_order=6814)

Removing basic block 5
long int bar ()
{
  struct o r;
  bool _1;
  long int _4;
  long int _7;

  <bb 2> [local count: 1073741824]:
  r = foo ();
  _1 = MEM[(const struct ob *)&r].payload.D.109140.engaged;
  if (_1 != 0)
    goto <bb 3>; [66.00%]
  else
    goto <bb 4>; [34.00%]

  <bb 3> [local count: 708669601]:
  _7 = MEM[(long int &)&r];

  <bb 4> [local count: 1073741824]:
  # _4 = PHI <_7(3), 0(2)>
  r ={v} {CLOBBER};
  return _4;

}

Which seems to be almost exactly identical to the one I get from the real
std::optional:

;; Function bar (_Z3barv, funcdef_no=6084, decl_uid=49565, cgraph_uid=5869,
symbol_order=5916)

Removing basic block 5
long int bar ()
{
  struct optional r;
  long int _1;
  bool _4;
  long int _5;

  <bb 2> [local count: 1073741824]:
  r = foo ();
  _4 = MEM[(const struct _Optional_base *)&r]._M_payload.D.50442._M_engaged;
  if (_4 != 0)
    goto <bb 3>; [66.00%]
  else
    goto <bb 4>; [34.00%]

  <bb 3> [local count: 708669601]:
  _5 = MEM[(long int &)&r];

  <bb 4> [local count: 1073741824]:
  # _1 = PHI <_5(3), 0(2)>
  r ={v} {CLOBBER};
  return _1;

}

Literally the only differences I can see is that variables are declared in a
different order, and that some variable names are different.

Yet the assembly output for my version optimizes the store to memory away just
fine, and the std::optional output still fails to optimize the store to memory.

Is the (very minor) difference here this significant or is there something I
can't see in the outputted GIMPLE that results in the differences ? I tried to
delve into the RTL, though I failed to really understand what was going on
(though I could see significant differences between what I wrote and the
original example there).
I've also checked the assembly, and as far as I can see, there is no functional
difference between what I wrote and the original one, LLVM even produces the
exact same assembly for both.

I've also tried to rule out the difference in variable declaration placement
and  naming by rewriting what I wrote into GIMPLE and modifying it to
correspond to the original example as well as possible, with this being my best
effort:

long int __GIMPLE (ssa,guessed_local(1073741824))
bar ()
{
  struct o r;
  long int _1;
  bool _4;
  long int _7;

  __BB(2,guessed_local(1073741824)):
  r = foo ();
  _4 = __MEM <const struct ob> ((const struct ob *)&r).payload.base.engaged;
  if (_4 != _Literal (bool) 0)
    goto __BB3(guessed(88583700));
  else
    goto __BB4(guessed(45634028));

  __BB(3,guessed_local(708669601)):
  _7 = __MEM <long int> (&r);
  goto __BB4(precise(134217728));

  __BB(4,guessed_local(1073741824)):
  _1 = __PHI (__BB3: _7, __BB2: 0l);
  r = _Literal (struct o) {};
  return _1;

}

But it still gets optimized well, as expected, unlike the original, which is
rather mind boggling to me, unless there really is a bunch of GIMPLE
information that isn't part of the outputted form.

PS: LLVM optimizes the original example and what I wrote perfectly fine to the
same assembly code.

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[glisse at gcc dot gnu.org]

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

--- Comment #3 from Marc Glisse <glisse at gcc dot gnu.org> ---
For a self-contained version, see below. Notice how the extra constructor in
_Optional_payload_base changes the generated code, or storing directly a
_Optional_payload_base instead of _Optional_payload in optional

struct _Optional_payload_base {
  long _M_value;
  bool _M_engaged = false;
  _Optional_payload_base() = default;
  ~_Optional_payload_base() = default;
  _Optional_payload_base(const _Optional_payload_base&) = default;
  _Optional_payload_base(_Optional_payload_base&&) = default;

  _Optional_payload_base(double,float);
};

struct _Optional_payload : _Optional_payload_base { };

struct optional
{
  _Optional_payload _M_payload;
};

optional foo();
long bar()
{
  auto r = foo();
  if (r._M_payload._M_engaged)
    return r._M_payload._M_value;
  else
    return 0L;
}

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[gabravier at gmail dot com]

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

--- Comment #4 from Gabriel Ravier <gabravier at gmail dot com> ---
Ah, I see. Didn't think there was a constructor involved and/or that GIMPLE
would keep it implicit like this...

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[glisse at gcc dot gnu.org]

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

--- Comment #5 from Marc Glisse <glisse at gcc dot gnu.org> ---
GIMPLE doesn't know about calling conventions, that's something that only
"appears" during expansion to RTL.
Still, I don't claim to understand what is going on here.

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cfsteefel at arista dot com

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 110648 has been marked as a duplicate of this bug. ***

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 101326 has been marked as a duplicate of this bug. ***

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vincenzo.innocente at cern dot ch

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 109281 has been marked as a duplicate of this bug. ***

[Bug rtl-optimization/95405] Unnecessary stores with std::optional

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |barry.revzin at gmail dot com

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 95383 has been marked as a duplicate of this bug. ***