[Bug tree-optimization/101885] New: wrong code at -O3 on x86_64-linux-gnu

[Bug tree-optimization/101885] New: wrong code at -O3 on x86_64-linux-gnu

[qrzhang at gatech dot edu]

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

            Bug ID: 101885
           Summary: wrong code at -O3 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: qrzhang at gatech dot edu
  Target Milestone: ---

It appears a regression in gcc-10. gcc-9 works fine.


$ gcc-trunk -v
gcc version 12.0.0 20210812 (experimental) [master revision
01f8a8b48e5:0eb7800d242:04b4f3152593f85b05974528d1607619dd77d702] (GCC)


$ gcc-trunk abc.c ; ./a.out
0

$ gcc-trunk -O3 abc.c ; ./a.out
3


$ cat abc.c
int a = 3, c;
short b = 5, d, f;
volatile short e;
int main() {
  f = 29;
  for (; f != 33; f++) {
    int g = a;
    if (b)
      if (a) {
        a = b = 0;
        for (; b <= 8; b++)
          ;
      }
    e;
    e;
    c = 0;
    for (; c != 14; ++c)
      e;
  }
  b = d;
  printf("", 8);
  printf("%d\n", a);
}

[Bug tree-optimization/101885] wrong code at -O3 on x86_64-linux-gnu

[qrzhang at gatech dot edu]

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

--- Comment #1 from Qirun Zhang <qrzhang at gatech dot edu> ---
My bisection points to g:529ea7d9596b26ba103578eeab448e9862a2d2c5

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.4
            Summary|wrong code at -O3 on        |[10/11/12 Regression] wrong
                   |x86_64-linux-gnu            |code at -O3 on
                   |                            |x86_64-linux-gnu

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-08-12
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.  The gimple level definitely looks correct.

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Qirun Zhang from comment #1)
> My bisection points to g:529ea7d9596b26ba103578eeab448e9862a2d2c5

r10-7268

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
                 CC|                            |law at gcc dot gnu.org
            Version|unknown                     |11.2.1

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Slightly cleaned up:
int a = 3, c;
short b = 5, d, f;
volatile short e;

__attribute__((noipa)) void
foo (void)
{
}

int
main ()
{
  for (f = 29; f != 33; f++)
    {
      int g = a;
      if (b)
        if (a)
          for (a = b = 0; b <= 8; b++)
            ;
      e;
      e;
      for (c = 0; c != 14; ++c)
        e;
    }
  b = d;
  foo ();
  if (a != 0)
    __builtin_abort ();
  return 0;
}

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And even
int a = 3, c;
short b = 5, d, f;
volatile short e;

__attribute__((noipa)) void
foo (void)
{
}

int
main ()
{
  for (f = 0; f != 2; f++)
    {
      int g = a;
      if (b)
        if (a)
          for (a = b = 0; b <= 3; b++)
            ;
      for (c = 0; c != 16; ++c)
        e;
    }
  b = d;
  foo ();
  if (a != 0)
    __builtin_abort ();
  return 0;
}

so one doesn't need to unroll that many times etc.

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
        setne   %sil
        andl    %esi, %edx
        xorl    %esi, %esi
        cmovne  %edi, %ecx
        cmovne  %esi, %eax
looks wrong, xorl will always set ZF.

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Seems it goes wrong during combine, before combine we have:
(insn 17 16 131 2 (parallel [
            (set (reg:QI 124 [ _199 ])
                (and:QI (reg:QI 143)
                    (reg:QI 145)))
            (clobber (reg:CC 17 flags))
        ]) "pr101885.c":17:5 534 {*andqi_1}
     (expr_list:REG_DEAD (reg:QI 145)
        (expr_list:REG_DEAD (reg:QI 143)
            (expr_list:REG_UNUSED (reg:CC 17 flags)
                (nil)))))
(insn 131 17 130 2 (set (reg:QI 152)
        (const_int 0 [0])) "pr101885.c":17:5 83 {*movqi_internal}
     (nil))
(insn 130 131 132 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg:QI 124 [ _199 ])
            (const_int 0 [0]))) "pr101885.c":17:5 5 {*cmpqi_ccno_1}
     (nil))
(insn 132 130 134 2 (set (reg:QI 82 [ b_lsm_flag.26 ])
        (if_then_else:QI (ne (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg:QI 124 [ _199 ])
            (reg:QI 152))) "pr101885.c":17:5 1204 {*movqicc_noc}
     (expr_list:REG_DEAD (reg:QI 152)
        (expr_list:REG_DEAD (reg:QI 124 [ _199 ])
            (expr_list:REG_EQUAL (if_then_else:QI (ne (reg:CCZ 17 flags)
                        (const_int 0 [0]))
                    (reg:QI 124 [ _199 ])
                    (const_int 0 [0]))
                (nil)))))
(insn 134 132 135 2 (set (reg:SI 153)
        (const_int 0 [0])) "pr101885.c":17:5 81 {*movsi_internal}
     (nil))
(insn 135 134 137 2 (set (reg:SI 90 [ a_lsm.27 ])
        (if_then_else:SI (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg:SI 90 [ a_lsm.27 ])
            (reg:SI 153))) "pr101885.c":17:5 1201 {*movsicc_noc}
     (expr_list:REG_EQUAL (if_then_else:SI (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg:SI 90 [ a_lsm.27 ])
            (const_int 0 [0]))
        (nil)))
and that is in RTL what the optimized dump had:
  _33 = _31 & _32;
  if (_33 != 0)
    goto <bb 7>; [13.50%]
  else
    goto <bb 6>; [86.50%]

  <bb 6> [local count: 12627325]:

  <bb 7> [local count: 14598063]:
  # b_lsm_flag.26_11 = PHI <_33(5), b_lsm_flag.26_202(6)>
  # a_lsm.27_45 = PHI <0(5), a_lsm.27_203(6)>
i.e. both the conditional moves consume flags reg set from comparison of x & y
result against 0.
But combine makes:
(note 17 16 131 2 NOTE_INSN_DELETED)
(note 131 17 130 2 NOTE_INSN_DELETED)
(insn 130 131 132 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (and:QI (reg:QI 143)
                (reg:QI 145))
            (const_int 0 [0]))) "pr101885.c":17:5 515 {*testqi_1_maybe_si}
     (nil))
(insn 132 130 134 2 (parallel [
            (set (reg:QI 82 [ b_lsm_flag.26 ])
                (and:QI (reg:QI 143)
                    (reg:QI 145)))
            (clobber (reg:CC 17 flags))
        ]) "pr101885.c":17:5 534 {*andqi_1}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (expr_list:REG_DEAD (reg:QI 145)
            (expr_list:REG_DEAD (reg:QI 143)
                (nil)))))
out of this (followed by the conditional moves).  That means it sets flags and
then immediately clobbers them by another and.
Ideally we want *andqi_2_maybe_si insn for both that
        (set (reg 17)
            (compare (and:QI (match_operand:QI 1 ("nonimmediate_operand")
("%0,0,0"))
                    (match_operand:QI 2 ("general_operand") ("qn,m,n")))
                (const_int 0 [0])))
        (set (match_operand:QI 0 ("nonimmediate_operand") ("=qm,q,r"))
            (and:QI (match_dup 1)
                (match_dup 2)))
but if that isn't used, at least the flag setting should be after 132 and not
before it...

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
We have:
Trying 17 -> 130:
   17: {r124:QI=r143:QI&r145:QI;clobber flags:CC;}
      REG_DEAD r145:QI
      REG_DEAD r143:QI
      REG_UNUSED flags:CC
  130: flags:CCZ=cmp(r124:QI,0)
Successfully matched this instruction:
(parallel [
        (set (reg:CCZ 17 flags)
            (compare:CCZ (and:QI (reg:QI 143)
                    (reg:QI 145))
                (const_int 0 [0])))
        (set (reg:QI 124 [ _199 ])
            (and:QI (reg:QI 143)
                (reg:QI 145)))
    ])
That merged what we wanted, nice!

And then the wrong:
Trying 130 -> 132:
  130: {flags:CCZ=cmp(r143:QI&r145:QI,0);r124:QI=r143:QI&r145:QI;}
      REG_DEAD r143:QI
      REG_DEAD r145:QI
  132: r82:QI={(flags:CCZ!=0)?r124:QI:r152:QI}
      REG_DEAD r152:QI
      REG_DEAD r124:QI
      REG_EQUAL {(flags:CCZ!=0)?r124:QI:0}
Failed to match this instruction:
(parallel [
        (set (reg:QI 82 [ b_lsm_flag.26 ])
            (and:QI (reg:QI 143)
                (reg:QI 145)))
        (set (reg:CCZ 17 flags)
            (compare:CCZ (and:QI (reg:QI 143)
                    (reg:QI 145))
                (const_int 0 [0])))
    ])
Failed to match this instruction:
(parallel [
        (set (reg:QI 82 [ b_lsm_flag.26 ])
            (and:QI (reg:QI 143)
                (reg:QI 145)))
        (set (reg:CCZ 17 flags)
            (compare:CCZ (and:QI (reg:QI 143)
                    (reg:QI 145))
                (const_int 0 [0])))
    ])
Successfully matched this instruction:
(set (reg:CCZ 17 flags)
    (compare:CCZ (and:QI (reg:QI 143)
            (reg:QI 145))
        (const_int 0 [0])))
Successfully matched this instruction:
(set (reg:QI 82 [ b_lsm_flag.26 ])
    (and:QI (reg:QI 143)
        (reg:QI 145)))
I guess it would be fine if (reg 17 flags) didn't have other users (in insn 135
in particular).

The failed match attempt
(parallel [
        (set (reg:QI 82 [ b_lsm_flag.26 ])
            (and:QI (reg:QI 143)
                (reg:QI 145)))
        (set (reg:CCZ 17 flags)
            (compare:CCZ (and:QI (reg:QI 143)
                    (reg:QI 145))
                (const_int 0 [0])))
    ])
actually looks almost good, except that it would need to try them in the other
order in the parallel.
I must say I forgot whether the flags first then operation ordering is now
canonical everywhere, or whether some backends want one and others another one.
But I vaguely remember there are various passes that only work with the
ordering x86 has.

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And the:
Successfully matched this instruction:
(set (reg:CCZ 17 flags)
    (compare:CCZ (and:QI (reg:QI 143)
            (reg:QI 145))
        (const_int 0 [0])))
Successfully matched this instruction:
(set (reg:QI 82 [ b_lsm_flag.26 ])
    (and:QI (reg:QI 143)
        (reg:QI 145)))
case should be rejected on the grounds that flags isn't dead at the last of
these insns and that clobbers of that reg are added when the second insn is
being recognized.

Segher, do you think you could have a look?

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For instructions that inherently set a condition code register, the
@code{compare} operator is always written as the first RTL expression of
the @code{parallel} instruction pattern.  For example,

@smallexample
(define_insn ""
  [(set (reg:CCZ FLAGS_REG)
        (compare:CCZ
          (plus:SI
            (match_operand:SI 1 "register_operand" "%r")
            (match_operand:SI 2 "register_operand" "r"))
          (const_int 0)))
   (set (match_operand:SI 0 "register_operand" "=r")
        (plus:SI (match_dup 1) (match_dup 2)))]
  ""
  "addl %0, %1, %2")
@end smallexample
see the
https://gcc.gnu.org/pipermail/gcc-patches/2017-May/thread.html#474080
thread.

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |roger at nextmovesoftware dot com
             Status|NEW                         |ASSIGNED
                 CC|                            |roger at nextmovesoftware dot com

--- Comment #11 from Roger Sayle <roger at nextmovesoftware dot com> ---
Patch proposed
https://gcc.gnu.org/pipermail/gcc-patches/2022-February/589741.html

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[segher at gcc dot gnu.org]

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

--- Comment #12 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #8)> The failed match attempt
> (parallel [
>         (set (reg:QI 82 [ b_lsm_flag.26 ])
>             (and:QI (reg:QI 143)
>                 (reg:QI 145)))
>         (set (reg:CCZ 17 flags)
>             (compare:CCZ (and:QI (reg:QI 143)
>                     (reg:QI 145))
>                 (const_int 0 [0])))
>     ])
> actually looks almost good, except that it would need to try them in the
> other order in the parallel.
> I must say I forgot whether the flags first then operation ordering is now
> canonical everywhere, or whether some backends want one and others another
> one.

It is canonical, and has been since literally forever.  81ad201ac5f6 (from
2017)
makes this explicit in our documentation.  compare-elim.c used to get this
wrong, but it was fixed in 4f0473fe89e6 (also 2017).

> But I vaguely remember there are various passes that only work with the
> ordering x86 has.

Only compare-elim did.  It was fixed.

[Bug rtl-optimization/101885] [10/11/12 Regression] wrong code at -O3 on x86_64-linux-gnu

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Roger Sayle <sayle@gcc.gnu.org>:

https://gcc.gnu.org/g:49365d511ac9b64009b1de11ef8a941f59407f67

commit r12-7046-g49365d511ac9b64009b1de11ef8a941f59407f67
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Fri Feb 4 09:32:21 2022 +0000

    [PATCH] PR rtl-optimization/101885: Prevent combine from clobbering flags

    This patch addresses PR rtl-optimization/101885 which is a P2 wrong code
    regression.  In combine, if the resulting fused instruction is a parallel
    of two sets which fails to be recognized by the backend, combine tries to
    emit these as two sequential set instructions (known as split_i2i3).
    As each set is recognized the backend may add any necessary "clobbers".
    The code currently checks that any clobbers added to the first "set"
    don't interfere with the second set, but doesn't currently handle the
    case that clobbers added to the second set may interfere/kill the
    destination of the first set (which must be live at this point).
    The solution is to cut'n'paste the "clobber" logic from just a few
    lines earlier, suitably adjusted for the second instruction.

    One minor nit that may confuse a reviewer is that at this point in
    the code we've lost track of which set was first and which was second
    (combine chooses dynamically, and the recog processes that adds the
    clobbers may have obfuscated the original SET_DEST) so the actual test
    below is to confirm that any newly added clobbers (on the second set
    instruction) don't overlap either set0's or set1's destination.

    2022-02-04  Roger Sayle  <roger@nextmovesoftware.com>

    gcc/ChangeLog
            PR rtl-optimization/101885
            * combine.cc (try_combine): When splitting a parallel into two
            sequential sets, check not only that the first doesn't clobber
            the second but also that the second doesn't clobber the first.

    gcc/testsuite/ChangeLog
            PR rtl-optimization/101885
            * gcc.dg/pr101885.c: New test case.

[Bug rtl-optimization/101885] [10/11 Regression] wrong code at -O3 on x86_64-linux-gnu

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[10/11/12 Regression] wrong |[10/11 Regression] wrong
                   |code at -O3 on              |code at -O3 on
                   |x86_64-linux-gnu            |x86_64-linux-gnu
      Known to work|                            |12.0

--- Comment #14 from Roger Sayle <roger at nextmovesoftware dot com> ---
Fixed on mainline.

[Bug rtl-optimization/101885] [10/11 Regression] wrong code at -O3 on x86_64-linux-gnu

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
           Assignee|roger at nextmovesoftware dot com  |unassigned at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #15 from Roger Sayle <roger at nextmovesoftware dot com> ---
Unassigning myself (in case anyone wants to backport this fix themselves).

[Bug rtl-optimization/101885] [10/11 Regression] wrong code at -O3 on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

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

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

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

[Bug rtl-optimization/101885] [10/11 Regression] wrong code at -O3 on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug rtl-optimization/101885] [11 Regression] wrong code at -O3 on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #17 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.