[Bug rtl-optimization/108292] New: [13 Regression] wrong code with vector compare & mask at -O1 and above

[Bug rtl-optimization/108292] New: [13 Regression] wrong code with vector compare & mask at -O1 and above

[zsojka at seznam dot cz]

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

            Bug ID: 108292
           Summary: [13 Regression] wrong code with vector compare & mask
                    at -O1 and above
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zsojka at seznam dot cz
  Target Milestone: ---
              Host: x86_64-pc-linux-gnu
            Target: x86_64-pc-linux-gnu

Created attachment 54191
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54191&action=edit
reduced testcase

Output:
$ x86_64-pc-linux-gnu-gcc -O testcase.c
$ ./a.out 
Aborted

(gdb) p/x x
$2 = {0x0, 0x0, 0x0, 0x0, 0x0, 0xfffffffa, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x0, 0x0, 0x0}

$ x86_64-pc-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/repo/gcc-trunk/binary-latest-amd64/bin/x86_64-pc-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/repo/gcc-trunk/binary-trunk-r13-5005-20230105111747-g72ce780a497-checking-yes-rtl-df-extra-nobootstrap-amd64/bin/../libexec/gcc/x86_64-pc-linux-gnu/13.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /repo/gcc-trunk//configure --enable-languages=c,c++
--enable-valgrind-annotations --disable-nls --enable-checking=yes,rtl,df,extra
--disable-bootstrap --with-cloog --with-ppl --with-isl
--build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu
--target=x86_64-pc-linux-gnu --with-ld=/usr/bin/x86_64-pc-linux-gnu-ld
--with-as=/usr/bin/x86_64-pc-linux-gnu-as --disable-libstdcxx-pch
--prefix=/repo/gcc-trunk//binary-trunk-r13-5005-20230105111747-g72ce780a497-checking-yes-rtl-df-extra-nobootstrap-amd64
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.0.0 20230105 (experimental) (GCC)

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
                 CC|                            |jakub at gcc dot gnu.org
   Target Milestone|---                         |13.0

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r13-4977-gd0558f420b2a5692fd38ac76ffa97ae6c1726ed9

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-01-05
     Ever confirmed|0                           |1

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The assembly difference is
-       movl    $1, x+20(%rip)
+       movl    $-6, x+20(%rip)
Now, that is the correct value to be stored into x[5] by the
__builtin_sub_overflow (0, 6, &x[5]);
statement, but each element is then shifted right by (element != 0) ? 31 : 0,
so in this case -6U >> 31 aka 1.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, we have ix86_expand_int_movcc called with operands:
(reg:SI 170)
(ne (reg:SI 86 [ _14 ])
    (const_int 0 [0]))
(const_int 31 [0x1f])
(const_int 0 [0])
and the function emits:
(insn 42 0 43 (set (reg:CC 17 flags)
        (compare:CC (reg:SI 86 [ _14 ])
            (const_int 1 [0x1]))) "pr108292.c":9:5 -1
     (nil))

(insn 43 42 44 (parallel [
            (set (reg:SI 171)
                (if_then_else:SI (ltu:SI (reg:CC 17 flags)
                        (const_int 0 [0]))
                    (const_int -1 [0xffffffffffffffff])
                    (const_int 0 [0])))
            (clobber (reg:CC 17 flags))
        ]) "pr108292.c":9:5 -1
     (expr_list:REG_EQUAL (neg:SI (ne:SI (reg:SI 86 [ _14 ])
                (const_int 0 [0])))
        (nil)))

(insn 44 43 45 (set (reg:SI 172)
        (not:SI (reg:SI 171))) "pr108292.c":9:5 -1
     (nil))

(insn 45 44 46 (parallel [
            (set (reg:SI 173)
                (and:SI (reg:SI 172)
                    (const_int 31 [0x1f])))
            (clobber (reg:CC 17 flags))
        ]) "pr108292.c":9:5 -1
     (nil))

(insn 46 45 0 (set (reg:SI 170)
        (reg:SI 173)) "pr108292.c":9:5 -1
     (nil))
I believe the REG_EQUAL note is incorrect, it would be correct on insn 44,
pseudo 172 is r86 != 0 ? -1 : 0, while pseudo 171 is r86 == 0 ? -1 : 0.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

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

--- Comment #4 from Roger Sayle <roger at nextmovesoftware dot com> ---
Doh! Apologies for the inconvenience.  I'll have a fix soon.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Though, it seems the REG_EQUAL note is also wrong on the cmov10.c testcase
which went with the commit (again, exact opposite).

Seems we have multiple cases where this REG_EQUAL note is newly added.
One is x != 0 ? -1 : y, negate_cc_compare_p is true and code overridden to EQ
and ct/cf swapped.
So, the note is then (neg (eq x const0_rtx)).
Another one is x == 0 ? y : -1, negate_cc_compare_p is true, code, ct and cf
unchanged.
The note is then (neg (eq x const0_rtx)) too.
The sequence emitted for both of these cases is neg{l,q} x; sbb{l,q} tmp, tmp,
which sets tmp to 0 if x == 0 and to -1 if x != 0.  So the right note would be
IMNSHO
(neg (ne x const0_rtx)) for both of the above cases.
Another case is the sign_bit_compare_p case, which happens for (signed)
x >= 0 ? y : z
x < 0 ? y : z
x > -1 ? y : z
x <= -1 ? y : z
The code handling those cases is:
              if (code == GT || code == GE)
                code = reverse_condition (code);
              else
                {
                  std::swap (ct, cf);
                  diff = ct - cf;
                }
              tmp = emit_store_flag (tmp, code, op0, op1, VOIDmode, 0, -1);
In that case emit_store_flag sets tmp to (code op0 op1) ? -1 : 0 so I think
(neg (code op0 op1)) is the right REG_EQUAL note for it.
The last case is when ix86_expand_carry_flag_compare returns true.
This is when LTU/GEU comparisons are used or something which can be turned into
one,
e.g. x == 0 or x != 0 turned into x < 1U or x >= 1U.

Now, the code below the addition of the new REG_EQUAL note assumes that the
comparison
set tmp to 0 if true and to -1 if false (which can be seen e.g. from diff == 1
&& ct == 0 case not adding any further instructions).  So the REG_EQUAL note
should be IMHO
gen_rtx_NEG (mode, gen_rtx_fmt_ee (reverse_condition (code), mode, op0, op1))
rather than gen_rtx_NEG (mode, gen_rtx_fmt_ee (code, mode, op0, op1))
for the cases where std::swap (ct, cf); has not been done (with the original
code, not the adjusted one).  For the sign_bit_compare_p case that matches the
above analysis that
it is handled correctly, as we reverse_condition iff swap isn't done.
For the negate_cc_compare_p case I think it matches the analysis too that it is
always wrong and should be always NE.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So I wonder about:
--- gcc/config/i386/i386-expand.cc.jj   2023-01-04 10:45:49.978883731 +0100
+++ gcc/config/i386/i386-expand.cc      2023-01-05 18:22:40.228518935 +0100
@@ -3271,10 +3271,12 @@ ix86_expand_int_movcc (rtx operands[])
            {
              negate_cc_compare_p = true;
              std::swap (ct, cf);
-             code = EQ;
            }
          else if (code == EQ && op1 == const0_rtx && cf == -1)
-           negate_cc_compare_p = true;
+           {
+             negate_cc_compare_p = true;
+             code = NE;
+           }
        }

       diff = ct - cf;
@@ -3319,11 +3321,7 @@ ix86_expand_int_movcc (rtx operands[])

              /* To simplify rest of code, restrict to the GEU case.  */
              if (compare_code == LTU)
-               {
-                 std::swap (ct, cf);
-                 compare_code = reverse_condition (compare_code);
-                 code = reverse_condition (code);
-               }
+               std::swap (ct, cf);
              else
                {
                  if (fpcmp)
@@ -3333,6 +3331,10 @@ ix86_expand_int_movcc (rtx operands[])
                  else
                    PUT_CODE (compare_op,
                              reverse_condition (GET_CODE (compare_op)));
+                 if (SCALAR_FLOAT_MODE_P (GET_MODE (op0)))
+                   code = reverse_condition_maybe_unordered (code);
+                 else
+                   code = reverse_condition (code);
                }
              diff = ct - cf;

but am not very sure about that, especially the floating point stuff, e.g.
whether
for -ffast-math it shouldn't actually be reverse_condition (and not add any
REG_EQUAL note if code is UNKNOWN?).

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Or for now don't add any REG_EQUAL note if op0 has scalar floating point
mode...

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[roger at nextmovesoftware dot com]

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

--- Comment #8 from Roger Sayle <roger at nextmovesoftware dot com> ---
Created attachment 54195
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54195&action=edit
Roger's proposed patch

Here's my proposed patch (or something close to it, it's still bootstrapping
and regression testing).  The goal is to capture the original comparison (at
the top of the BB), and avoid/ignore the actual comparison that we may have
converted to.
Sorry again to the inconvenience.  Please let me know what you think.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[roger at nextmovesoftware dot com]

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

--- Comment #9 from Roger Sayle <roger at nextmovesoftware dot com> ---
Another way to avoid the SCALAR_FLOAT_MODE_P problem is:
          /* Add a REG_EQUAL note to allow condition to be shared.  */
          rtx note = gen_rtx_fmt_ee (orig_code, mode, op0, op1);
          /* TMP is (orig_code op0 op1) ? 0 : -1, or swapped.  */
          note = orig_swapped ? gen_rtx_NEG (mode, node)
                              : gen_rtx_PLUS (mode, note, constm1_rtx);
          set_unique_reg_note (get_last_insn (), REG_EQUAL, note);

i.e. don't attempt to modify the initial comparison; instead tweak the note.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Roger Sayle from comment #8)
> Created attachment 54195 [details]
> Roger's proposed patch
> 
> Here's my proposed patch (or something close to it, it's still bootstrapping
> and regression testing).  The goal is to capture the original comparison (at
> the top of the BB), and avoid/ignore the actual comparison that we may have
> converted to.
> Sorry again to the inconvenience.  Please let me know what you think.

I don't like that much (of course, Uros is the maintainer so his call).
But, I think we should avoid bundling the code generation changes (the dropping
of ct or cf checks for negate_cc_compare_p; if that is a win, it should be
proven separately but it isn't clear if it is) and the patch just causes
further dead variables which the current code already has many.
What is the point of orig_code when code is already dead except for this newly
added REG_EQUAL note?  Why can't just code contain it?
As for the floating point stuff, yes, the - 1 would be an option, but perhaps
should be used only if we need to reverse floating point comparison?
If we introduce a bool for swap_ct_cf, then it seems pointless to also actually
swap ct/cf and recompute diff in all the spots, just set swap_ct_cf and
swap/recompute diff only if the bool is set and we need to look at ct/cf/diff.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[ubizjak at gmail dot com]

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

--- Comment #11 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Jakub Jelinek from comment #10)
> (In reply to Roger Sayle from comment #8)
> > Here's my proposed patch (or something close to it, it's still bootstrapping
> > and regression testing).  The goal is to capture the original comparison (at
> > the top of the BB), and avoid/ignore the actual comparison that we may have
> > converted to.
> > Sorry again to the inconvenience.  Please let me know what you think.

The fix is not a simple patch, and it looks to me that the original patch has
several rough edges. I propose to revert the original patch at this stage of
development and retry with it at the next stage 1.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 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:9e6ac747ac5cff9e3f58421cdd9f03538e48ed07

commit r13-5038-g9e6ac747ac5cff9e3f58421cdd9f03538e48ed07
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Thu Jan 5 19:49:45 2023 +0000

    [Committed] PR rtl-optimization/108292: Revert "Improve
ix86_expand_int_movcc to allow condition (mask) sharing"

    This reverts commit d0558f420b2a5692fd38ac76ffa97ae6c1726ed9.

    2023-01-05  Roger Sayle  <roger@nextmovesoftware.com>

    gcc/ChangeLog
            PR rtl-optimization/108292
            * config/i386/i386-expand.cc (ix86_expand_int_movcc): Revert
            previous changes.

    gcc/testsuite/ChangeLog
            PR rtl-optimization/108292
            * gcc.target/i386/cmov10.c: Remove test case.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[hjl.tools at gmail dot com]

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

--- Comment #13 from H.J. Lu <hjl.tools at gmail dot com> ---
I also got

$ cat foo.i
extern void foo (float *);

extern int x;
int
mouse_frame_side(void) {
  float mouseloc;
  foo (&mouseloc);
  return mouseloc > x ? 1 : 2;
}
$ gcc -march=alderlake -Ofast -S foo.i
during RTL pass: cse2
foo.i: In function ?mouse_frame_side?:
foo.i:9:1: internal compiler error: in copy_rtx, at rtl.cc:369
    9 | }
      | ^
0x148a75b copy_rtx(rtx_def*)
        ../../src-master/gcc/rtl.cc:369
0x148a690 copy_rtx(rtx_def*)
        ../../src-master/gcc/rtl.cc:342
0x2b72758 cse_insn
        ../../src-master/gcc/cse.cc:4562
0x2b787b8 cse_extended_basic_block
        ../../src-master/gcc/cse.cc:6566
0x2b78cd6 cse_main
        ../../src-master/gcc/cse.cc:6711
0x2b7ae5e rest_of_handle_cse2
        ../../src-master/gcc/cse.cc:7600
0x2b7af84 execute
        ../../src-master/gcc/cse.cc:7655
Please submit a full bug report, with preprocessed source (by using
-freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.             

(gdb) f 3
#3  0x0000000002b72759 in cse_insn (insn=0x7fffea01bac0)
    at ../../src-master/gcc/cse.cc:4562
4562            src_eqv = copy_rtx (XEXP (tem, 0));
(gdb) call debug_rtx (insn)
(insn 48 47 49 2 (parallel [
            (set (reg:SI 94)
                (if_then_else:SI (unlt:SI (reg:CCFP 17 flags)
                        (const_int 0 [0]))
                    (const_int -1 [0xffffffffffffffff])
                    (const_int 0 [0])))
            (clobber (reg:CC 17 flags))
        ]) "/export/project/git/gcc-bisect/foo.i":8:27 discrim 2 1288
{*x86_movsicc_0_m1}
     (expr_list:REG_DEAD (reg:CCFP 17 flags)
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (expr_list:REG_EQUAL (neg:SI (UnKnown:SI Unknown))
                (nil)))))
(gdb)

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[roger at nextmovesoftware dot com]

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

--- Comment #14 from Roger Sayle <roger at nextmovesoftware dot com> ---
Created attachment 54197
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54197&action=edit
Related optimizations to ix86_expand_int_movcc.

Just for the record, here is a related patch that I was working on (touching
the problematic piece of code), before everything went pear shaped.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[roger at nextmovesoftware dot com]

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

Roger Sayle <roger at nextmovesoftware dot com> changed:

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

--- Comment #15 from Roger Sayle <roger at nextmovesoftware dot com> ---
This regression has now been resolved by reverting the problematic patch. 
Sorry for the inconvenience.  Perhaps we'll have better luck (with a better
solution) after we return to stage 1.

[Bug rtl-optimization/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zhendong.su at inf dot ethz.ch

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
*** Bug 108308 has been marked as a duplicate of this bug. ***

[Bug target/108292] [13 Regression] wrong code with vector compare & mask at -O1 and above

[cvs-commit at gcc dot gnu.org]

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

--- Comment #17 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:b990e80e39b41ec028d018eac7d2d040a537b681

commit r13-5043-gb990e80e39b41ec028d018eac7d2d040a537b681
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 6 10:52:34 2023 +0100

    testsuite: Add testcases from PR108292 and PR108308

    These PRs were for now fixed by reversion of the r13-4977
    patch, but so that the problems don't reappear during stage 1,
    I'm adding testcase coverage from those PRs.

    2023-01-06  Jakub Jelinek  <jakub@redhat.com>

            PR target/108292
            PR target/108308
            * gcc.c-torture/execute/pr108292.c: New test.
            * gcc.target/i386/pr108292.c: New test.
            * gcc.dg/pr108308.c: New test.