[Bug tree-optimization/94524] New: wrong code with vector modulo operation

[Bug tree-optimization/94524] New: wrong code with vector modulo operation

[zsojka at seznam dot cz]

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

            Bug ID: 94524
           Summary: wrong code with vector modulo operation
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zsojka at seznam dot cz
  Target Milestone: ---
              Host: x86_64-pc-linux-gnu

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

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

I am not sure sure the testcase has defined behavior, but I checked
https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html#Integers-implementation
and https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html and I didn't find
any indication that vector operations should behave differently than scalars.
Behavior is the same with -fwrapv and neither -ftrapv or -fsanitize=undefined
don't report any problem.

$ 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-r10-7581-20200407082749-gbee27152f7e-checking-yes-rtl-df-extra-nobootstrap-amd64/bin/../libexec/gcc/x86_64-pc-linux-gnu/10.0.1/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-r10-7581-20200407082749-gbee27152f7e-checking-yes-rtl-df-extra-nobootstrap-amd64
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.0.1 20200407 (experimental) (GCC)

[Bug tree-optimization/94524] [8/9/10 Regression] wrong code with vector modulo operation since r0-117580

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|wrong code with vector      |[8/9/10 Regression] wrong
                   |modulo operation            |code with vector modulo
                   |                            |operation since r0-117580
                 CC|                            |jakub at gcc dot gnu.org
   Target Milestone|---                         |8.5
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-04-07

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with my r0-117580-g4ee4c52c64cc1eeda53aae6e221b5b1bd9bd7421 so I
believe this should work correctly in 4.7 (and 4.6 didn't accept it).
The vect pattern code tried to follow what the expander does, but in that case
there is match.pd rule that canonicalizes x % -C into x % C.  Will need to dig
up if something in the expansion tweaks the sign of the divisor or if it has
the same bug as the vect pattern.

[Bug tree-optimization/94524] [8/9/10 Regression] wrong code with vector modulo operation since r0-117580

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
4527                    /* n rem d = n rem -d */
4528                    if (rem_flag && d < 0)
4529                      {
4530                        d = abs_d;
4531                        op1 = gen_int_mode (abs_d, int_mode);
4532                      }
in expmed.c indeed canonicalizes this.

[Bug tree-optimization/94524] [8/9/10 Regression] wrong code with vector modulo operation since r0-117580

[rguenth at gcc dot gnu.org]

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

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

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

[Bug tree-optimization/94524] [8/9/10 Regression] wrong code with vector modulo operation since r0-117580

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Both the tree-vect-patterns.c and tree-vect-generic.c code contains the same.
Except that the latter which handles modulo with a VECTOR_CST divisor, where we
properly divide by the absolute value, but when computing the modulo, we
actually use the original VECTOR_CST to multiply again.
So,
--- gcc/tree-vect-generic.c.jj  2020-01-12 11:54:38.518381650 +0100
+++ gcc/tree-vect-generic.c     2020-04-08 09:02:56.465316469 +0200
@@ -478,6 +478,7 @@ expand_vector_divmod (gimple_stmt_iterat
 {
   bool use_pow2 = true;
   bool has_vector_shift = true;
+  bool use_abs_op1 = false;
   int mode = -1, this_mode;
   int pre_shift = -1, post_shift;
   unsigned int nunits = nunits_for_known_piecewise_op (type);
@@ -618,7 +619,10 @@ expand_vector_divmod (gimple_stmt_iterat

          /* n rem d = n rem -d */
          if (code == TRUNC_MOD_EXPR && d < 0)
-           d = abs_d;
+           {
+             d = abs_d;
+             use_abs_op1 = true;
+           }
          else if (abs_d == HOST_WIDE_INT_1U << (prec - 1))
            {
              /* This case is not handled correctly below.  */
@@ -899,6 +903,23 @@ expand_vector_divmod (gimple_stmt_iterat
   if (op == unknown_optab
       || optab_handler (op, TYPE_MODE (type)) == CODE_FOR_nothing)
     return NULL_TREE;
+  if (use_abs_op1)
+    {
+      tree_vector_builder elts;
+      if (!elts.new_unary_operation (type, op1, false))
+        return NULL_TREE;
+      unsigned int count = elts.encoded_nelts ();
+      for (unsigned int i = 0; i < count; ++i)
+        {
+          tree elem1 = VECTOR_CST_ELT (op1, i);
+        
+          tree elt = const_unop (ABS_EXPR, TREE_TYPE (elem1), elem1);
+          if (elt == NULL_TREE)
+            return NULL_TREE;
+          elts.quick_push (elt);
+        }
+      op1 = elts.build ();
+    }
   tem = gimplify_build2 (gsi, MULT_EXPR, type, cur_op, op1);
   op = optab_for_tree_code (MINUS_EXPR, type, optab_default);
   if (op == unknown_optab
should fix this (the last hunk because const_binop apparently doesn't handle
ABS_EXPR on VECTOR_CST :(, and it is probably not appropriate time to add it
now).

That said, I'll need to verify also what we do for modulo with the signed
minimum divisor (either all elts or just one).

[Bug tree-optimization/94524] [8/9/10 Regression] wrong code with vector modulo operation since r0-117580

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 48239
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48239&action=edit
gcc10-pr94524.patch

Full untested fix.

[Bug tree-optimization/94524] [8/9/10 Regression] wrong code with vector modulo operation since r0-117580

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 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:f52eb4f988992d393c69ee4ab76f236dced80e36

commit r10-7639-gf52eb4f988992d393c69ee4ab76f236dced80e36
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 8 21:22:05 2020 +0200

    vect: Fix up lowering of TRUNC_MOD_EXPR by negative constant [PR94524]

    The first testcase below is miscompiled, because for the division part
    of the lowering we canonicalize negative divisors to their absolute value
    (similarly how expmed.c canonicalizes it), but when multiplying the
division
    result back by the VECTOR_CST, we use the original constant, which can
    contain negative divisors.

    Fixed by computing ABS_EXPR of the VECTOR_CST.  Unfortunately, fold-const.c
    doesn't support const_unop (ABS_EXPR, VECTOR_CST) and I think it is too
late
    in GCC 10 cycle to add it now.

    Furthermore, while modulo by most negative constant happens to return the
    right value, it does that only by invoking UB in the IL, because
    we then expand division by that 1U+INT_MAX and say for INT_MIN % INT_MIN
    compute the division as -1, and then multiply by INT_MIN, which is signed
    integer overflow.  We in theory could do the computation in unsigned vector
    types instead, but is it worth bothering.  People that are doing % INT_MIN
    are either testing for standard conformance, or doing something wrong.
    So, I've also added punting on % INT_MIN, both in vect lowering and vect
    pattern recognition (we punt already for / INT_MIN).

    2020-04-08  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/94524
            * tree-vect-generic.c (expand_vector_divmod): If any elt of op1 is
            negative for signed TRUNC_MOD_EXPR, multiply with absolute value of
            op1 rather than op1 itself at the end.  Punt for signed modulo by
            most negative constant.
            * tree-vect-patterns.c (vect_recog_divmod_pattern): Punt for signed
            modulo by most negative constant.

            * gcc.c-torture/execute/pr94524-1.c: New test.
            * gcc.c-torture/execute/pr94524-2.c: New test.

[Bug tree-optimization/94524] [8/9 Regression] wrong code with vector modulo operation since r0-117580

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[8/9/10 Regression] wrong   |[8/9 Regression] wrong code
                   |code with vector modulo     |with vector modulo
                   |operation since r0-117580   |operation since r0-117580

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug tree-optimization/94524] [8/9 Regression] wrong code with vector modulo operation since r0-117580

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:780b2ab071391495690141c61604370d6cf7af49

commit r9-8877-g780b2ab071391495690141c61604370d6cf7af49
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 8 21:22:05 2020 +0200

    vect: Fix up lowering of TRUNC_MOD_EXPR by negative constant [PR94524]

    The first testcase below is miscompiled, because for the division part
    of the lowering we canonicalize negative divisors to their absolute value
    (similarly how expmed.c canonicalizes it), but when multiplying the
division
    result back by the VECTOR_CST, we use the original constant, which can
    contain negative divisors.

    Fixed by computing ABS_EXPR of the VECTOR_CST.  Unfortunately, fold-const.c
    doesn't support const_unop (ABS_EXPR, VECTOR_CST) and I think it is too
late
    in GCC 10 cycle to add it now.

    Furthermore, while modulo by most negative constant happens to return the
    right value, it does that only by invoking UB in the IL, because
    we then expand division by that 1U+INT_MAX and say for INT_MIN % INT_MIN
    compute the division as -1, and then multiply by INT_MIN, which is signed
    integer overflow.  We in theory could do the computation in unsigned vector
    types instead, but is it worth bothering.  People that are doing % INT_MIN
    are either testing for standard conformance, or doing something wrong.
    So, I've also added punting on % INT_MIN, both in vect lowering and vect
    pattern recognition (we punt already for / INT_MIN).

    2020-04-08  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/94524
            * tree-vect-generic.c (expand_vector_divmod): If any elt of op1 is
            negative for signed TRUNC_MOD_EXPR, multiply with absolute value of
            op1 rather than op1 itself at the end.  Punt for signed modulo by
            most negative constant.
            * tree-vect-patterns.c (vect_recog_divmod_pattern): Punt for signed
            modulo by most negative constant.

            * gcc.c-torture/execute/pr94524-1.c: New test.
            * gcc.c-torture/execute/pr94524-2.c: New test.

    (cherry picked from commit f52eb4f988992d393c69ee4ab76f236dced80e36)

[Bug tree-optimization/94524] [8/9 Regression] wrong code with vector modulo operation since r0-117580

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 8.5 in r8-10486-g7146b8fd63e5107f1cf896df92fbaed99aa5ac0d and for
9.4+ by the above commit too.