[Bug c/101066] New: Wrong code after fixup_cfg3

[Bug c/101066] New: Wrong code after fixup_cfg3

[stefansf at linux dot ibm.com]

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

            Bug ID: 101066
           Summary: Wrong code after fixup_cfg3
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: stefansf at linux dot ibm.com
  Target Milestone: ---
            Target: s390*-*-*, x86_64-*-*

int a = 1, c, d, e;
int *b = &a;
static int g(int *h) {
  c = *h;
  return d;
}
static void f(int *h) {
  e = *h;
  *b = 0;
  g(h);
}
int main() {
  f(b);
  printf("%d\n", c);
}

Running `gcc t.c -Os && ./a.out` results in printed 1 whereas 0 is expected.
This does not happen for -O[0,1,2,3] i.e. there 0 is printed.

Prior fixup_cfg3 the code looks good to me and afterwards the assignment to c
uses a cached/initial value of variable a which is wrong:

int main ()
{
  int * b.0_1;
  int c.1_2;
  int _6;
  int _7;
  int * b.2_8;
  int _10;
  int _11;

  <bb 2> [local count: 1073741824]:
  b.0_1 = b;
  _6 = *b.0_1;
  _7 = _6;
  e = _7;
  b.2_8 = b;
  *b.2_8 = 0;
  _10 = _6;
  c = _10;
  _11 = d;
  c.1_2 = c;
  printf ("%d\n", c.1_2);
  return 0;

}

Reproducible on IBM Z as well as x86_64 using commit 831589c227c.

[Bug tree-optimization/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Wrong code after fixup_cfg3 |[10/11/12 Regression] Wrong
                   |                            |code after fixup_cfg3 since
                   |                            |r10-3311-gff6686d2e5f797d6
     Ever confirmed|0                           |1
   Target Milestone|---                         |10.4
                 CC|                            |jamborm at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
   Last reconfirmed|                            |2021-06-15
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Confirmed, started with r10-3311-gff6686d2e5f797d6.

[Bug tree-optimization/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug tree-optimization/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[jamborm at gcc dot gnu.org]

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

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 #2 from Martin Jambor <jamborm at gcc dot gnu.org> ---
The IPA-SRA transform is indeed invalid.  There is a thinko in the point where
safe_to_import_accesses check is overruled.  So mine.

[Bug ipa/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[jamborm at gcc dot gnu.org]

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

--- Comment #3 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I have proposed a fix on the mailing list:
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573338.html

[Bug ipa/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[stefansf at linux dot ibm.com]

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

--- Comment #4 from Stefan Schulze Frielinghaus <stefansf at linux dot ibm.com> ---
(In reply to Martin Jambor from comment #3)
> I have proposed a fix on the mailing list:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573338.html

I gave it a try on IBM Z where the testcase runs fine, now. Thanks!

[Bug ipa/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS 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:763121ccd908f52bc666f277ea2cf42110b3aad9

commit r12-2172-g763121ccd908f52bc666f277ea2cf42110b3aad9
Author: Martin Jambor <mjambor@suse.cz>
Date:   Thu Jul 8 19:44:41 2021 +0200

    ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066)

    The "new" IPA-SRA has a more difficult job than the previous
    not-truly-IPA version when identifying situations in which a parameter
    passed by reference can be passed into a third function and only thee
    converted to one passed by value (and possibly "split" at the same
    time).

    In order to allow this, two conditions must be fulfilled.  First the
    call to the third function must happen before any modifications of
    memory, because it could change the value passed by reference.
    Second, in order to make sure we do not introduce new (invalid)
    dereferences, the call must postdominate the entry BB.

    The second condition is actually not necessary if the caller function
    is also certain to dereference the pointer but the first one must
    still hold.  Unfortunately, the code making this overriding decision
    also happen to trigger when the first condition is not fulfilled.
    This is fixed in the following patch.

    gcc/ChangeLog:

    2021-06-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/101066
            * ipa-sra.c (class isra_call_summary): New member
            m_before_any_store, initialize it in the constructor.
            (isra_call_summary::dump): Dump the new field.
            (ipa_sra_call_summaries::duplicate): Copy it.
            (process_scan_results): Set it.
            (isra_write_edge_summary): Stream it.
            (isra_read_edge_summary): Likewise.
            (param_splitting_across_edge): Only override
            safe_to_import_accesses if m_before_any_store is set.

    gcc/testsuite/ChangeLog:

    2021-06-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/101066
            * gcc.dg/ipa/pr101066.c: New test.

[Bug ipa/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:6745246120d18db857344894967f2dc9c586fd0a

commit r11-8712-g6745246120d18db857344894967f2dc9c586fd0a
Author: Martin Jambor <mjambor@suse.cz>
Date:   Fri Jul 9 16:09:53 2021 +0200

    ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066)

    The "new" IPA-SRA has a more difficult job than the previous
    not-truly-IPA version when identifying situations in which a parameter
    passed by reference can be passed into a third function and only thee
    converted to one passed by value (and possibly "split" at the same
    time).

    In order to allow this, two conditions must be fulfilled.  First the
    call to the third function must happen before any modifications of
    memory, because it could change the value passed by reference.
    Second, in order to make sure we do not introduce new (invalid)
    dereferences, the call must postdominate the entry BB.

    The second condition is actually not necessary if the caller function
    is also certain to dereference the pointer but the first one must
    still hold.  Unfortunately, the code making this overriding decision
    also happen to trigger when the first condition is not fulfilled.
    This is fixed in the following patch.

    gcc/ChangeLog:

    2021-06-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/101066
            * ipa-sra.c (class isra_call_summary): New member
            m_before_any_store, initialize it in the constructor.
            (isra_call_summary::dump): Dump the new field.
            (ipa_sra_call_summaries::duplicate): Copy it.
            (process_scan_results): Set it.
            (isra_write_edge_summary): Stream it.
            (isra_read_edge_summary): Likewise.
            (param_splitting_across_edge): Only override
            safe_to_import_accesses if m_before_any_store is set.

    gcc/testsuite/ChangeLog:

    2021-06-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/101066
            * gcc.dg/ipa/pr101066.c: New test.

    (cherry picked from commit 763121ccd908f52bc666f277ea2cf42110b3aad9)

[Bug ipa/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r10-9993-g5aa28c8cf15cd254cc5a3a12278133b93b8b017f
Author: Martin Jambor <mjambor@suse.cz>
Date:   Mon Jul 19 20:18:43 2021 +0200

    ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066)

    The "new" IPA-SRA has a more difficult job than the previous
    not-truly-IPA version when identifying situations in which a parameter
    passed by reference can be passed into a third function and only thee
    converted to one passed by value (and possibly "split" at the same
    time).

    In order to allow this, two conditions must be fulfilled.  First the
    call to the third function must happen before any modifications of
    memory, because it could change the value passed by reference.
    Second, in order to make sure we do not introduce new (invalid)
    dereferences, the call must postdominate the entry BB.

    The second condition is actually not necessary if the caller function
    is also certain to dereference the pointer but the first one must
    still hold.  Unfortunately, the code making this overriding decision
    also happen to trigger when the first condition is not fulfilled.
    This is fixed in the following patch.

    gcc/ChangeLog:

    2021-06-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/101066
            * ipa-sra.c (class isra_call_summary): New member
            m_before_any_store, initialize it in the constructor.
            (isra_call_summary::dump): Dump the new field.
            (ipa_sra_call_summaries::duplicate): Copy it.
            (process_scan_results): Set it.
            (isra_write_edge_summary): Stream it.
            (isra_read_edge_summary): Likewise.
            (param_splitting_across_edge): Only override
            safe_to_import_accesses if m_before_any_store is set.

    gcc/testsuite/ChangeLog:

    2021-06-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/101066
            * gcc.dg/ipa/pr101066.c: New test.

    (cherry picked from commit 763121ccd908f52bc666f277ea2cf42110b3aad9)

[Bug ipa/101066] [10/11/12 Regression] Wrong code after fixup_cfg3 since r10-3311-gff6686d2e5f797d6

[jamborm at gcc dot gnu.org]

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

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

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

--- Comment #8 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Eventually I decided to backport the full fix to gcc-10 too (as opposed to
bailing out more aggresively).  So fixed also on all affected branches.