[Bug tree-optimization/108166] New: [12/13 Regression] Wrong code with -O2

[Bug tree-optimization/108166] New: [12/13 Regression] Wrong code with -O2

[vsevolod.livinskiy at gmail dot com]

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

            Bug ID: 108166
           Summary: [12/13 Regression] Wrong code with -O2
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vsevolod.livinskiy at gmail dot com
  Target Milestone: ---

Link to the Compiler Explorer: https://godbolt.org/z/j8coqj375

Reproducer:
#include <stdio.h>
bool a, b;
int d, c;
const int &e(const int &f, const int &g) { return !f ? f : g; }
int main() {
  c = e(b, 0) > ((b ? d : b) ?: 8);
  a = b ? d : b;
  printf("%d\n", a);
  if (a != 0)
    __builtin_abort();
}

Error:
>$ g++ -O2 driver.cpp && ./a.out 
1
Aborted (core dumped)
>$ g++ -O0 driver.cpp && ./a.out 
0

gcc version 13.0.0 20221216 (2fdc8546b5c6cb1fe254e40b5bdd19ed6fbb44da)

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3
   Last reconfirmed|                            |2022-12-18
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.
  # iftmp.2_17 = PHI <iftmp.2_24(5), 0(4)>
  if (iftmp.2_17 != 0)
    goto <bb 8>; [INV]
  else
    goto <bb 7>; [INV]

  <bb 7> :

  <bb 8> :
  # iftmp.1_16 = PHI <iftmp.2_17(6), 8(7)>
  _7 = _4 > iftmp.1_16;
  _8 = (int) _7;
  c = _8;
  D.3392 ={v} {CLOBBER(eol)};
  D.3393 ={v} {CLOBBER(eol)};
  _11 = iftmp.2_17 != 0;

And then _11 gets replaced with:
  _6 = iftmp.2_10 != 0;

I can't figure out why though.

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection
           Priority|P3                          |P2

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|needs-bisection             |
            Summary|[12/13 Regression] Wrong    |[12/13 Regression] Wrong
                   |code with -O2               |code with -O2 since
                   |                            |r12-8078-ga42aa68bf1ad745a
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r12-8078-ga42aa68bf1ad745a.

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Testcase without includes:
bool a, b;
int d, c;

const int &
foo (const int &f, const int &g)
{
  return !f ? f : g;
}

__attribute__((noipa)) void
bar (int)
{
}

int
main ()
{
  c = foo (b, 0) > ((b ? d : b) ?: 8);
  a = b ? d : b;
  bar (a);
  if (a != 0)
    __builtin_abort ();
}

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The phiopt2 optimization is:
   # iftmp.3_10 = PHI <iftmp.3_14(3), 0(2)>
-  if (iftmp.3_10 != 0)
-    goto <bb 6>; [56.25%]
-  else
-    goto <bb 5>; [43.75%]
-
-  <bb 5> [local count: 536870913]:
-
-  <bb 6> [local count: 1073741824]:
-  # iftmp.2_9 = PHI <iftmp.3_10(4), 8(5)>
-  _4 = iftmp.2_9 < 0;
+  _4 = iftmp.3_10 < 0;

So, we have _4 = (iftmp.3_10 ?: 8) < 0 and the optimization optimizes that to
_4 = iftmp.3_10 < 0.  If iftmp.3_10 is non-zero, then it is the same
comparison,
if iftmp.3_10 is zero, then previously we'd set _4 to 8 < 0 and newly set it to
0 < 0, both are false.
The problem is in global ranges I think, previously we had:
  # RANGE [irange] int [-INF, -1][1, +INF]
  # iftmp.2_9 = PHI <iftmp.3_10(4), 8(5)>
which is correct, it was iftmp.3_10 ?: 8 which is always non-zero.
But this rnage is moved to iftmp.3_10 after the optimization:
  # RANGE [irange] int [-INF, -1][1, +INF]
  # iftmp.3_10 = PHI <iftmp.3_14(3), 0(2)>
which is incorrect, we don't know anything about iftmp.3_10 range there because
d is VARYING.

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[rguenth at gcc dot gnu.org]

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
But we end up with

 <bb 2> [local count: 1073741824]:
  b.1_1 = b;
  if (b.1_1 != 0)
    goto <bb 3>; [50.00%]
  else
    goto <bb 4>; [50.00%]

  <bb 3> [local count: 536870913]:
  iftmp.3_14 = d;

  <bb 4> [local count: 966367640]:
  # RANGE [irange] int [-INF, -1][1, +INF]
  # iftmp.3_10 = PHI <iftmp.3_14(3), 0(2)>

  <bb 6> [local count: 1073741824]:
  # RANGE [irange] int [-INF, -1][1, +INF]
  # iftmp.2_9 = PHI <iftmp.3_10(4)>
  _4 = iftmp.2_9 < 0;

before CFG cleanup which does look wrong.  Before the phiopt we have

  <bb 4> [local count: 966367640]:
  # iftmp.3_10 = PHI <iftmp.3_14(3), 0(2)>
  if (iftmp.3_10 != 0)
    goto <bb 6>; [56.25%]
  else 
    goto <bb 5>; [43.75%]

  <bb 5> [local count: 536870913]:

  <bb 6> [local count: 1073741824]:
  # RANGE [irange] int [-INF, -1][1, +INF]
  # iftmp.2_9 = PHI <iftmp.3_10(4), 8(5)>
  _4 = iftmp.2_9 < 0;

but clearly value-replacing the edge 5->6 with iftmp.3_10 is wrong (its
zero, not 8 on that edge)?

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
replace_phi_edge_with_variable assumes we replace things with the same value,
if the new transform does something different because of the _uses_ of the
value then it has to make sure to not copy range info in that function (add
another argument to it?)

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54142
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54142&action=edit
gcc13-pr108166.patch

Untested fix.  I wouldn't change replace_phi_edge_with_variable, because even
without the duplicate_ssa_name_range_info (say if new_tree already has
SSA_NAME_RANGE_INFO) we'd be lying to the compiler.
The right thing would be to union the global range of the phi result with the
oarg INTEGER_CST, not sure how hard would that be.

[Bug tree-optimization/108166] [12/13 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:5c17adfb5d08e34da7a7f234dfc2ed1f0aaadaa9

commit r13-4845-g5c17adfb5d08e34da7a7f234dfc2ed1f0aaadaa9
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Dec 22 12:52:48 2022 +0100

    phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

    The following place in value_replacement is after proving that
    x == cst1 ? cst2 : x
    phi result is only used in a comparison with constant which doesn't
    care if it compares cst1 or cst2 and replaces it with x.
    The testcase is miscompiled because we have after the replacement
    incorrect range info for the phi result, we would need to
    effectively union the phi result range with cst1 (oarg in the code)
    because previously that constant might be missing in the range, but
    newly it can appear (we've just verified that the single use stmt
    of the phi result doesn't care about that value in particular).

    The following patch just resets the info, bootstrapped/regtested
    on x86_64-linux and i686-linux, ok for trunk?

    Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO
    with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO
    (including adjusting non-zero bits and the like)?

    2022-12-22  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/108166
            * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p
            case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result.

            * g++.dg/torture/pr108166.C: New test.

[Bug tree-optimization/108166] [12 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12/13 Regression] Wrong    |[12 Regression] Wrong code
                   |code with -O2 since         |with -O2 since
                   |r12-8078-ga42aa68bf1ad745a  |r12-8078-ga42aa68bf1ad745a

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug tree-optimization/108166] [12 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:86d252ab555d487aefb616562e770ffa46e05b01

commit r12-9133-g86d252ab555d487aefb616562e770ffa46e05b01
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Thu Dec 22 12:52:48 2022 +0100

    phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]

    The following place in value_replacement is after proving that
    x == cst1 ? cst2 : x
    phi result is only used in a comparison with constant which doesn't
    care if it compares cst1 or cst2 and replaces it with x.
    The testcase is miscompiled because we have after the replacement
    incorrect range info for the phi result, we would need to
    effectively union the phi result range with cst1 (oarg in the code)
    because previously that constant might be missing in the range, but
    newly it can appear (we've just verified that the single use stmt
    of the phi result doesn't care about that value in particular).

    The following patch just resets the info, bootstrapped/regtested
    on x86_64-linux and i686-linux, ok for trunk?

    Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO
    with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO
    (including adjusting non-zero bits and the like)?

    2022-12-22  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/108166
            * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p
            case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result.

            * g++.dg/torture/pr108166.C: New test.

    (cherry picked from commit 5c17adfb5d08e34da7a7f234dfc2ed1f0aaadaa9)

[Bug tree-optimization/108166] [12 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for gcc 12.3 too.

[Bug tree-optimization/108166] [12 Regression] Wrong code with -O2 since r12-8078-ga42aa68bf1ad745a

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jiajing_zheng at 163 dot com

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