[Bug tree-optimization/112616] New: wrong code at -O{s,2,3} on x86_64-linux-gnu

[Bug tree-optimization/112616] New: wrong code at -O{s,2,3} on x86_64-linux-gnu

[zhendong.su at inf dot ethz.ch]

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

            Bug ID: 112616
           Summary: wrong code at -O{s,2,3} on x86_64-linux-gnu
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zhendong.su at inf dot ethz.ch
  Target Milestone: ---

It appears to be a regression from 9.*, and affects 10.* and later.

Compiler Explorer: https://godbolt.org/z/GxdzdqTjd


[510] % gcctk -v
Using built-in specs.
COLLECT_GCC=gcctk
COLLECT_LTO_WRAPPER=/local/suz-local/software/local/gcc-trunk/libexec/gcc/x86_64-pc-linux-gnu/14.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-trunk/configure --disable-bootstrap
--enable-checking=yes --prefix=/local/suz-local/software/local/gcc-trunk
--enable-sanitizers --enable-languages=c,c++ --disable-werror --enable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 14.0.0 20231118 (experimental) (GCC) 
[511] % 
[511] % gcctk -O1 small.c; ./a.out
[512] % 
[512] % gcctk -O3 small.c
[513] % ./a.out
Segmentation fault
[514] % 
[514] % cat small.c
unsigned a;
int b, d, e, f = 2, g, h = 1, *i = &b;
volatile int c = 1;
static int *o() {
  long m = ~a;
  int j = f / b, k = f - 1, n = m << -1 / ~g / k;
  if (j && n)
    c;
  return &e;
}
static long p() {
  int *q = 0, **r = &q;
  if (c) {
    *i = h;
    *r = o();
  }
  return *q;
}
int main() {
  p();
  int *l = 0;
  if (d)
    c = *l;
  return 0;
}

[Bug tree-optimization/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
            Summary|wrong code at -O{s,2,3} on  |[11/12/13/14 Regression]
                   |x86_64-linux-gnu            |wrong code at -O{s,2,3} on
                   |                            |x86_64-linux-gnu
   Target Milestone|---                         |11.5

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-11-19
                 CC|                            |marxin at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
          Component|tree-optimization           |ipa

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
  # q_11 = PHI <0B(2), removed_return.14_14(D)(4), removed_return.14_14(D)(3)>
  _12 = *q_11;


WTF????

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is like PR 108007 but unlike that one, -fno-tree-dce is not used.

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jamborm at gcc dot gnu.org
            Version|unknown                     |13.2.1

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
IPA-SRA at work.  With -fno-inline added you see

-long int p ()
+void p.isra ()
 {
...
@@ -106,13 +82,13 @@
   i.2_2 = i;
   h.3_3 = h;
   *i.2_2 = h.3_3;
-  _4 = o ();
+  o.isra ();

   <bb 4> [local count: 1073741824]:
-  # q_11 = PHI <0B(2), _4(3)>
-  _5 = *q_11;
-  _10 = (long int) _5;
-  return _10;
+  # q_5 = PHI <0B(2), removed_return.17_12(D)(3)>
+  _6 = *q_5;
+  _7 = (long int) _6;
+  return;


Note the issue is that we end up with the following after inlining:

  <bb 5> [local count: 1073741824]:
  # q_11 = PHI <0B(2), removed_return.14_14(D)(4), removed_return.14_14(D)(3)>
  _12 = *q_11;

and CCP will optimistically simplify q_11 to 0B because accessing the
removed_return.14_14(D) would be undefined behavior.

That means substituting a default-def isn't correct - it exposes undefined
behavior on valid paths of the program.

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
            Summary|[11/12/13/14 Regression]    |[11/12/13/14 Regression]
                   |wrong code at -O{s,2,3} on  |wrong code at -O{s,2,3} on
                   |x86_64-linux-gnu            |x86_64-linux-gnu since
                   |                            |r10-3311

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r10-3311-gff6686d2e5f797d6c6a36ad14a7084bc1dc350e4

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

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

--- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> This is like PR 108007 but unlike that one, -fno-tree-dce is not used.

But the patch fixes it, so I gess it's time to make it pass ppc64le bootstrap.

(But I did not want to backport that patch, I wonder whether we can't figure
out something simpler :-/ )

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[jamborm at gcc dot gnu.org]

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

--- Comment #6 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
>   # q_11 = PHI <0B(2), removed_return.14_14(D)(4),
> removed_return.14_14(D)(3)>
>   _12 = *q_11;
> 
> 
> WTF????

Well, _12 is not used anywhere, so the code expects the entire load to be DCEd.
 But it gets optimized to 

  _2 = MEM[(int *)0B]; 

before DCE sees it and then even if _2 is never used anywhere, apparently the
statement is kept there as an intended trap (I guess).

I have adjusted my patch to make DCE for removed returnd part of IPA edge
redirection so that it does not have compare-debug problems and submitted it
for review in: https://inbox.sourceware.org/gcc-patches/ri6cyu1e9kw.fsf@/T/#u

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Jambor <jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:a9a8426e534760b8d3a250e9bd3cff4db131a2be

commit r14-8398-ga9a8426e534760b8d3a250e9bd3cff4db131a2be
Author: Martin Jambor <mjambor@suse.cz>
Date:   Wed Jan 24 19:12:31 2024 +0100

    ipa: Self-DCE of uses of removed call LHSs (PR 108007)

    PR 108007 is another manifestation where we rely on DCE to clean-up
    after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
    can leave behind statements which are fed uninitialized values and
    trap, even though their results are themselves never used.

    I have already fixed this for unused parameters in callees, this bug
    shows that almost the same thing can happen for removed returns, on
    the side of callers.  This means that the issue has to be fixed
    elsewhere, in call redirection.  This patch adds a function which
    looks for (and through, using a work-list) uses of operations fed
    specific SSA names and removes them all.

    That would have been easy if it wasn't for debug statements during
    tree-inline (from which call redirection is also invoked).  Debug
    statements are decoupled from the rest at this point and iterating
    over uses of SSAs does not bring them up.  During tree-inline they are
    handled especially at the end, I assume in order to make sure that
    relative ordering of UIDs are the same with and without debug info.

    This means that during tree-inline we need to make a hash of killed
    SSAs, that we already have in copy_body_data, available to the
    function making the purging.  So the patch duly does also that, making
    the interface slightly ugly.  Moreover, all newly unused SSA names
    need to be freed and as PR 112616 showed, it must be done in a defined
    order, which is what newly added ipa_release_ssas_in_hash does.

    gcc/ChangeLog:

    2024-01-12  Martin Jambor  <mjambor@suse.cz>

            PR ipa/108007
            PR ipa/112616
            * cgraph.h (cgraph_edge): Add a parameter to
            redirect_call_stmt_to_callee.
            * ipa-param-manipulation.h (ipa_param_adjustments): Add a
            parameter to modify_call.
            (ipa_release_ssas_in_hash): Declare.
            * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
            parameter killed_ssas, pass it to padjs->modify_call.
            * ipa-param-manipulation.cc (purge_all_uses): New function.
            (ipa_param_adjustments::modify_call): New parameter killed_ssas.
            Instead of substituting uses, invoke purge_all_uses.  If
            hash of killed SSAs has not been provided, create a temporary one
            and release SSAs that have been added to it.
            (compare_ssa_versions): New function.
            (ipa_release_ssas_in_hash): Likewise.
            * tree-inline.cc (redirect_all_calls): Create
            id->killed_new_ssa_names earlier, pass it to edge redirection,
            adjust a comment.
            (copy_body): Release SSAs in id->killed_new_ssa_names.

    gcc/testsuite/ChangeLog:

    2024-01-15  Martin Jambor  <mjambor@suse.cz>

            PR ipa/108007
            PR ipa/112616
            * gcc.dg/ipa/pr108007.c: New test.
            * gcc.dg/ipa/pr112616.c: Likewise.

[Bug ipa/112616] [11/12/13/14 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[jamborm at gcc dot gnu.org]

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

--- Comment #8 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Fixed on trunk.  I did not want to backport this but because this variant does
not require disabling DCE, I will probably do after a few weeks on master, if
there are no issues.

[Bug ipa/112616] [11/12/13 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Martin Jambor
<jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:40ddc0b05a47f999b24f20c1becb79004995731b

commit r13-8594-g40ddc0b05a47f999b24f20c1becb79004995731b
Author: Martin Jambor <mjambor@suse.cz>
Date:   Mon Apr 8 17:34:33 2024 +0200

    ipa: Self-DCE of uses of removed call LHSs (PR 108007)

    PR 108007 is another manifestation where we rely on DCE to clean-up
    after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
    can leave behind statements which are fed uninitialized values and
    trap, even though their results are themselves never used.

    I have already fixed this for unused parameters in callees, this bug
    shows that almost the same thing can happen for removed returns, on
    the side of callers.  This means that the issue has to be fixed
    elsewhere, in call redirection.  This patch adds a function which
    looks for (and through, using a work-list) uses of operations fed
    specific SSA names and removes them all.

    That would have been easy if it wasn't for debug statements during
    tree-inline (from which call redirection is also invoked).  Debug
    statements are decoupled from the rest at this point and iterating
    over uses of SSAs does not bring them up.  During tree-inline they are
    handled especially at the end, I assume in order to make sure that
    relative ordering of UIDs are the same with and without debug info.

    This means that during tree-inline we need to make a hash of killed
    SSAs, that we already have in copy_body_data, available to the
    function making the purging.  So the patch duly does also that, making
    the interface slightly ugly.  Moreover, all newly unused SSA names
    need to be freed and as PR 112616 showed, it must be done in a defined
    order, which is what newly added ipa_release_ssas_in_hash does.

    This backport to gcc-13 also contains
    54e505d0446f86b7ad383acbb8e5501f20872b64 in order not to reintroduce
    PR 113757.

    gcc/ChangeLog:

    2024-04-05  Martin Jambor  <mjambor@suse.cz>

            PR ipa/108007
            PR ipa/112616
            * cgraph.h (cgraph_edge): Add a parameter to
            redirect_call_stmt_to_callee.
            * ipa-param-manipulation.h (ipa_param_adjustments): Add a
            parameter to modify_call.
            (ipa_release_ssas_in_hash): Declare.
            * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
            parameter killed_ssas, pass it to padjs->modify_call.
            * ipa-param-manipulation.cc (purge_all_uses): New function.
            (ipa_param_adjustments::modify_call): New parameter killed_ssas.
            Instead of substituting uses, invoke purge_all_uses.  If
            hash of killed SSAs has not been provided, create a temporary one
            and release SSAs that have been added to it.
            (compare_ssa_versions): New function.
            (ipa_release_ssas_in_hash): Likewise.
            * tree-inline.cc (redirect_all_calls): Create
            id->killed_new_ssa_names earlier, pass it to edge redirection,
            adjust a comment.
            (copy_body): Release SSAs in id->killed_new_ssa_names.

    gcc/testsuite/ChangeLog:

    2024-01-15  Martin Jambor  <mjambor@suse.cz>

            PR ipa/108007
            PR ipa/112616
            * gcc.dg/ipa/pr108007.c: New test.
            * gcc.dg/ipa/pr112616.c: Likewise.

    (cherry picked from commit a9a8426e534760b8d3a250e9bd3cff4db131a2be)

[Bug ipa/112616] [11/12 Regression] wrong code at -O{s,2,3} on x86_64-linux-gnu since r10-3311

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Martin Jambor
<jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:2183e5b5aa3a080624cb95a06993e34dedd09cb2

commit r12-10443-g2183e5b5aa3a080624cb95a06993e34dedd09cb2
Author: Martin Jambor <mjambor@suse.cz>
Date:   Mon Apr 8 17:34:33 2024 +0200

    ipa: Self-DCE of uses of removed call LHSs (PR 108007)

    PR 108007 is another manifestation where we rely on DCE to clean-up
    after IPA-SRA and if the user explicitely switches DCE off, IPA-SRA
    can leave behind statements which are fed uninitialized values and
    trap, even though their results are themselves never used.

    I have already fixed this for unused parameters in callees, this bug
    shows that almost the same thing can happen for removed returns, on
    the side of callers.  This means that the issue has to be fixed
    elsewhere, in call redirection.  This patch adds a function which
    looks for (and through, using a work-list) uses of operations fed
    specific SSA names and removes them all.

    That would have been easy if it wasn't for debug statements during
    tree-inline (from which call redirection is also invoked).  Debug
    statements are decoupled from the rest at this point and iterating
    over uses of SSAs does not bring them up.  During tree-inline they are
    handled especially at the end, I assume in order to make sure that
    relative ordering of UIDs are the same with and without debug info.

    This means that during tree-inline we need to make a hash of killed
    SSAs, that we already have in copy_body_data, available to the
    function making the purging.  So the patch duly does also that, making
    the interface slightly ugly.  Moreover, all newly unused SSA names
    need to be freed and as PR 112616 showed, it must be done in a defined
    order, which is what newly added ipa_release_ssas_in_hash does.

    This backport to gcc-13 also contains
    54e505d0446f86b7ad383acbb8e5501f20872b64 in order not to reintroduce
    PR 113757.

    gcc/ChangeLog:

    2024-04-05  Martin Jambor  <mjambor@suse.cz>

            PR ipa/108007
            PR ipa/112616
            * cgraph.h (cgraph_edge): Add a parameter to
            redirect_call_stmt_to_callee.
            * ipa-param-manipulation.h (ipa_param_adjustments): Add a
            parameter to modify_call.
            (ipa_release_ssas_in_hash): Declare.
            * cgraph.cc (cgraph_edge::redirect_call_stmt_to_callee): New
            parameter killed_ssas, pass it to padjs->modify_call.
            * ipa-param-manipulation.cc (purge_all_uses): New function.
            (ipa_param_adjustments::modify_call): New parameter killed_ssas.
            Instead of substituting uses, invoke purge_all_uses.  If
            hash of killed SSAs has not been provided, create a temporary one
            and release SSAs that have been added to it.
            (compare_ssa_versions): New function.
            (ipa_release_ssas_in_hash): Likewise.
            * tree-inline.cc (redirect_all_calls): Create
            id->killed_new_ssa_names earlier, pass it to edge redirection,
            adjust a comment.
            (copy_body): Release SSAs in id->killed_new_ssa_names.

    gcc/testsuite/ChangeLog:

    2024-01-15  Martin Jambor  <mjambor@suse.cz>

            PR ipa/108007
            PR ipa/112616
            * gcc.dg/ipa/pr108007.c: New test.
            * gcc.dg/ipa/pr112616.c: Likewise.

    (cherry picked from commit 40ddc0b05a47f999b24f20c1becb79004995731b)