[Bug target/106590] New: x86-64 miscompilation starting with "i386: Improve ix86_expand_int_movcc" w/ mtune=skylake

[Bug target/106590] New: x86-64 miscompilation starting with "i386: Improve ix86_expand_int_movcc" w/ mtune=skylake

[andres at anarazel dot de]

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

            Bug ID: 106590
           Summary: x86-64 miscompilation starting with "i386: Improve
                    ix86_expand_int_movcc" w/ mtune=skylake
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: andres at anarazel dot de
  Target Milestone: ---

Hi,

The attached reproducer shows a miscompilation I found building postgres. I've
bisected postgres' failure to 1ceddd7497e, but it's of course possible it's
just surfacing a prior issue.

In my reproducer, and in postgres, the problem only occurs with -mtune=skylake
or higher, but I'm not sure how related that actually is.

$ /home/andres/build/gcc-master/install/bin/gcc --version
gcc (GCC) 12.0.1 20220423 (experimental)
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gcc-11 --version
gcc-11 (Debian 11.3.0-5) 11.3.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ /home/andres/build/gcc-master/install/bin/gcc -Wall -Wextra -O1
-mtune=skylake /tmp/test.i -o /tmp/test
$ /tmp/test
wrong results: procform->prorettype: 23, restype: 20

$ /home/andres/build/gcc-master/install/bin/gcc -Wall -Wextra -O1
-mtune=broadwell /tmp/test.i -o /tmp/test
$ /tmp/test
everything ok: procform->prorettype: 23, restype: 23

$ gcc-11 -Wall -Wextra -O1 -mtune=skylake /tmp/test.i -o /tmp/test
$ /tmp/test
everything ok: procform->prorettype: 23, restype: 23

I think it's pretty obvious that the code should never be able to result in
restype == 20.

Regards,

Andres

[Bug target/106590] x86-64 miscompilation starting with "i386: Improve ix86_expand_int_movcc" w/ mtune=skylake

[andres at anarazel dot de]

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

--- Comment #1 from Andres Freund <andres at anarazel dot de> ---
Created attachment 53441
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53441&action=edit
reproducer

[Bug target/106590] [12/13 Regression] x86-64 miscompilation starting with "i386: Improve ix86_expand_int_movcc" w/ mtune=skylake

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.2

[Bug target/106590] [12/13 Regression] x86-64 miscompilation starting with "i386: Improve ix86_expand_int_movcc" w/ mtune=skylake

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
r12-8233-g1ceddd7497e15d

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-08-12
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
          Component|target                      |rtl-optimization

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
CE goes bad:
IF-CASE-2 found, start 6, else 8
verify found no changes in insn with uid = 35.
changing bb of uid 5
  from 8 to 6
deleting block 8
Conversion succeeded on pass 1.

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So the IR is slightly different entering CE1. The two BB for the sides of the
if are swapped. But that is the only difference.

This is definitely a latent bug that got exposed, it is not the first latent
bug in CE1 recently either.

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 53442
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53442&action=edit
testcase that should be ready for gcc.c-torture/execute

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
      Known to fail|                            |12.1.0
                 CC|                            |jakub at gcc dot gnu.org

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Cleaned up testcase:

/* { dg-additional-options "-mtune=skylake" { target { i?86-*-* x86_64-*-* } }
} */

typedef struct A { short a; } A;
typedef A *B;
typedef struct C { int c, d; } C;
typedef C *D;

B
foo (void)
{
  static A r = { .a = 1 };
  return &r;
}

D
bar (void)
{
  static C r = { .c = 1, .d = 23 };
  return &r;
}

static inline int __attribute__((always_inline))
baz (short a)
{
  int e = 1, f;
  short g;
  D h;

  switch (a)
    {
    case 1:
      f = 23;
      g = 1;
      break;
    case 2:
      f = 20;
      g = 2;
      break;
    }

  h = bar ();

  if (h->d != f || h->c != g)
    __builtin_abort ();
  return e;
}

int
qux (void)
{
  B i = foo ();
  int e = 1;

  switch (i->a)
    {
    case 1:
    case 2:
      e = baz (i->a);
      break;
    case 3:
      e = 0;
      break;
    }

  return e;
}

int
main ()
{
  qux ();
  return 0;
}

I think this is a latent bug in noce_convert_multiple_sets_1.
For cond
(eq (reg/v:HI 82 [ g ])
    (const_int 1 [0x1]))
and cc_cmp
(eq (reg:CCZ 17 flags)
    (const_int 0 [0]))
when we try_emit_cmove_seq for
(insn 3 35 4 7 (set (reg/v:HI 82 [ g ])
        (const_int 2 [0x2])) "pr106590.c":37:9 84 {*movhi_internal}
     (nil))
insn, we get seq1
(insn 65 0 66 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:HI 82 [ g ])
            (const_int 1 [0x1]))) -1
     (nil))

(insn 66 65 67 (set (reg:QI 96)
        (ne:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) -1
     (nil))

(insn 67 66 68 (set (reg:HI 95)
        (zero_extend:HI (reg:QI 96))) -1
     (nil))

(insn 68 67 0 (parallel [
            (set (reg:HI 95)
                (plus:HI (reg:HI 95)
                    (const_int 1 [0x1])))
            (clobber (reg:CC 17 flags))
        ]) -1
     (nil))
and seq2
(insn 69 0 70 (set (reg:HI 97)
        (const_int 2 [0x2])) -1
     (nil))

(insn 70 69 0 (set (reg:HI 95)
        (if_then_else:HI (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg/v:HI 82 [ g ])
            (reg:HI 97))) -1
     (nil))
and later for
(insn 4 3 36 7 (set (reg/v:SI 84 [ f ])
        (const_int 20 [0x14])) "pr106590.c":36:9 83 {*movsi_internal}
     (nil))
we get seq1
(insn 72 0 71 (set (reg:SI 98)
        (const_int 20 [0x14])) -1
     (nil))

(insn 71 72 73 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:HI 82 [ g ])
            (const_int 1 [0x1]))) -1
     (nil))

(insn 73 71 0 (set (reg/v:SI 84 [ f ])
        (if_then_else:SI (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg/v:SI 84 [ f ])
            (reg:SI 98))) -1
     (nil))
and seq2
(insn 74 0 75 (set (reg:SI 99)
        (const_int 20 [0x14])) -1
     (nil))

(insn 75 74 0 (set (reg/v:SI 84 [ f ])
        (if_then_else:SI (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg/v:SI 84 [ f ])
            (reg:SI 99))) -1
     (nil))
In the former case, cost1 == cost2, in the latter case cost1 > cost2.
So, we for the first insn choose seq1 and for the second seq2.
That is wrong, because the first seq1 clobbers the (reg 17 flags) register
which is used in cc_cmp/rev_cc_cmp.  So, if we pick that seq1, we may not use
any seq2 afterwards for the latter instructions, need to always select seq1
since then.

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[jakub at gcc dot gnu.org]

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

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

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

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

Untested fix.

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[cvs-commit at gcc dot gnu.org]

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

--- 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:3a74a7bf62f47ed0d19866576378724be932ee17

commit r13-2042-g3a74a7bf62f47ed0d19866576378724be932ee17
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Aug 15 13:56:57 2022 +0200

    ifcvt: Fix up noce_convert_multiple_sets [PR106590]

    The following testcase is miscompiled on x86_64-linux.
    The problem is in the noce_convert_multiple_sets optimization.
    We essentially have:
    if (g == 1)
      {
        g = 1;
        f = 23;
      }
    else
      {
        g = 2;
        f = 20;
      }
    and for each insn try to create a conditional move sequence.
    There is code to detect overlap with the regs used in the condition
    and the destinations, so we actually try to construct:
    tmp_g = g == 1 ? 1 : 2;
    f = g == 1 ? 23 : 20;
    g = tmp_g;
    which is fine.  But, we actually try to create two different
    conditional move sequences in each case, seq1 with the whole
    (eq (reg/v:HI 82 [ g ]) (const_int 1 [0x1]))
    condition and seq2 with cc_cmp
    (eq (reg:CCZ 17 flags) (const_int 0 [0]))
    to rely on the earlier present comparison.  In each case, we
    compare the rtx costs and choose the cheaper sequence (seq1 if both
    have the same cost).
    The problem is that with the skylake tuning,
    tmp_g = g == 1 ? 1 : 2;
    is actually expanded as
    tmp_g = (g == 1) + 1;
    in seq1 (which clobbers (reg 17 flags)) and as a cmov in seq2
    (which doesn't).  The tuning says both have the same cost, so we
    pick seq1.  Next we check sequences for
    f = g == 1 ? 23 : 20; and here the seq2 cmov is cheaper, but it
    uses (reg 17 flags) which has been clobbered earlier.

    The following patch fixes that by detecting if we in the chosen
    sequence clobber some register mentioned in cc_cmp or rev_cc_cmp,
    and if yes, arranges for only seq1 (i.e. sequences that emit the
    comparison itself) to be used after that.

    2022-08-15  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/106590
            * ifcvt.cc (check_for_cc_cmp_clobbers): New function.
            (noce_convert_multiple_sets_1): If SEQ sets or clobbers any regs
            mentioned in cc_cmp or rev_cc_cmp, don't consider seq2 for any
            further conditional moves.

            * gcc.dg/torture/pr106590.c: New test.

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far, 12.2 is frozen now and I think the fix needs some
time on the trunk before backporting, so probably only will be fixed for 12.3.

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 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:030063c43f30a2335d3c03182df0beb82d003816

commit r12-8720-g030063c43f30a2335d3c03182df0beb82d003816
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Aug 15 13:56:57 2022 +0200

    ifcvt: Fix up noce_convert_multiple_sets [PR106590]

    The following testcase is miscompiled on x86_64-linux.
    The problem is in the noce_convert_multiple_sets optimization.
    We essentially have:
    if (g == 1)
      {
        g = 1;
        f = 23;
      }
    else
      {
        g = 2;
        f = 20;
      }
    and for each insn try to create a conditional move sequence.
    There is code to detect overlap with the regs used in the condition
    and the destinations, so we actually try to construct:
    tmp_g = g == 1 ? 1 : 2;
    f = g == 1 ? 23 : 20;
    g = tmp_g;
    which is fine.  But, we actually try to create two different
    conditional move sequences in each case, seq1 with the whole
    (eq (reg/v:HI 82 [ g ]) (const_int 1 [0x1]))
    condition and seq2 with cc_cmp
    (eq (reg:CCZ 17 flags) (const_int 0 [0]))
    to rely on the earlier present comparison.  In each case, we
    compare the rtx costs and choose the cheaper sequence (seq1 if both
    have the same cost).
    The problem is that with the skylake tuning,
    tmp_g = g == 1 ? 1 : 2;
    is actually expanded as
    tmp_g = (g == 1) + 1;
    in seq1 (which clobbers (reg 17 flags)) and as a cmov in seq2
    (which doesn't).  The tuning says both have the same cost, so we
    pick seq1.  Next we check sequences for
    f = g == 1 ? 23 : 20; and here the seq2 cmov is cheaper, but it
    uses (reg 17 flags) which has been clobbered earlier.

    The following patch fixes that by detecting if we in the chosen
    sequence clobber some register mentioned in cc_cmp or rev_cc_cmp,
    and if yes, arranges for only seq1 (i.e. sequences that emit the
    comparison itself) to be used after that.

    2022-08-15  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/106590
            * ifcvt.cc (check_for_cc_cmp_clobbers): New function.
            (noce_convert_multiple_sets_1): If SEQ sets or clobbers any regs
            mentioned in cc_cmp or rev_cc_cmp, don't consider seq2 for any
            further conditional moves.

            * gcc.dg/torture/pr106590.c: New test.

    (cherry picked from commit 3a74a7bf62f47ed0d19866576378724be932ee17)

[Bug rtl-optimization/106590] [12/13 Regression] x86-64 miscompilation starting with r12-8233-g1ceddd7497e15d w/ mtune=skylake

[jakub at gcc dot gnu.org]

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

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

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

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