[Bug tree-optimization/55761] New: process_assignment assumes -1 can be created

[Bug tree-optimization/55761] New: process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

             Bug #: 55761
           Summary: process_assignment assumes -1 can be created
    Classification: Unclassified
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: paulo@matos-sorge.com


Created attachment 29013
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29013
breaks GCC4.7.2 x86_64

Hello,

process_assignment in tree-tailcall.c assumes constant -1 can be created in any
mode (that matches all the conditions up until this point) in the line:
*m = build_int_cst (TREE_TYPE (non_ass_var), -1);

If, however, TREE_TYPE of non_ass_var of vector type for example, an ICE
occurs.

The following example breaks my port, x86_64 and probably more:

typedef short V4H __attribute__ ((vector_size (8)));
extern V4H __fp_rep_h (unsigned short B);

V4H
fn1 ()
{
    V4H vuq = __fp_rep_h (0);
    vuq -= __fp_rep_h (1);
    return vuq;
}

In processing assignment vuq = vuq - vuq0; (where vuq0 = __fp_rep_h (1)), we
try to create multiplier -1 with type V4H and that fails in build_int_cst_wide.

We need to check if it's of integral type and only ten apply build_int_cst.
Otherwise we should use fold_build1.

I tested GCC 4.7.2:
$ gcc -O2 baaclc_block.i 
baaclc_block.i: In function 'fn1':
baaclc_block.i:10:1: internal compiler error: in build_int_cst_wide, at
tree.c:1222
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

$ $ gcc -v
Using built-in specs.
COLLECT_GCC=/tools/oss/packages/x86_64-rhel5/gcc/4.7.2/bin/gcc
COLLECT_LTO_WRAPPER=/tools/oss/packages/x86_64-rhel5/gcc/4.7.2/libexec/gcc/x86_64-unknown-linux-gnu/4.7.2/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../../gcc-4.7.2/configure
--prefix=/tools/oss/packages/x86_64-rhel5/gcc/4.7.2 --with-gnu-as
--with-as=/tools/oss/packages/x86_64-rhel5/binutils/default/bin/as
--with-gnu-ld
--with-ld=/tools/oss/packages/x86_64-rhel5/binutils/default/bin/ld
--with-mpc=/tools/oss/packages/x86_64-rhel5/mpc/0.8.1
--with-mpfr=/tools/oss/packages/x86_64-rhel5/mpfr/2.4.2
--with-gmp=/tools/oss/packages/x86_64-rhel5/gmp/4.3.2
--enable-languages=c,c++,objc,fortran --enable-symvers=gnu
Thread model: posix
gcc version 4.7.2 (GCC)

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #1 from Paulo J. Matos <paulo@matos-sorge.com> 2012-12-20 15:53:48 UTC ---
This happens for the negate_expr case too in the same switch. 

I have a patch to fix this that I will upload momentarily.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-12-20
     Ever Confirmed|0                           |1

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> 2012-12-20 15:55:22 UTC ---
Confirmed.  It already handles floats.  Easiest would be to add a
build_minus_one_cst helper in tree.c alongside build_one_cst.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #3 from Paulo J. Matos <paulo@matos-sorge.com> 2012-12-20 16:01:23 UTC ---
Created attachment 29014
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29014
Use built_int_cst only for integral types, otherwise use fold_build1

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #4 from Paulo J. Matos <paulo@matos-sorge.com> 2012-12-20 16:58:08 UTC ---
I created a new patch from your comment to gcc-patches:
diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index 5b1fd2b..8c7d142 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -326,26 +326,14 @@ process_assignment (gimple stmt, gimple_stmt_iterator
call
       return true;

     case NEGATE_EXPR:
-      if (FLOAT_TYPE_P (TREE_TYPE (op0)))
-        *m = build_real (TREE_TYPE (op0), dconstm1);
-      else
-        *m = build_int_cst (TREE_TYPE (op0), -1);
-
+      *m = fold_unary (NEGATE_EXPR, TREE_TYPE (op0), op0);
       *ass_var = dest;
       return true;

     case MINUS_EXPR:
-      if (*ass_var == op0)
-        *a = fold_build1 (NEGATE_EXPR, TREE_TYPE (non_ass_var), non_ass_var);
-      else
-        {
-          if (FLOAT_TYPE_P (TREE_TYPE (non_ass_var)))
-            *m = build_real (TREE_TYPE (non_ass_var), dconstm1);
-          else
-            *m = build_int_cst (TREE_TYPE (non_ass_var), -1);
-
-          *a = fold_build1 (NEGATE_EXPR, TREE_TYPE (non_ass_var),
non_ass_var);
-        }
+        *a = fold_unary (NEGATE_EXPR, TREE_TYPE (non_ass_var), non_ass_var);
+      if (*ass_var == op1)
+        *m = fold_unary (NEGATE_EXPR, TREE_TYPE (non_ass_var), non_ass_var);

       *ass_var = dest;
       return true;


However, I am less confident that it works now. Mainly because of the *m in
MINUS_EXPR. It seems from the comments in tailcall structure that m should be
-1, not (NEGATE non_ass_var). However, we cannot really create a -1 for vectors
straightforwardly.

I guess that, as per your comment below, we would need a build_minus_one_cst
that handles not only scalar types but also vector types.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #5 from Paulo J. Matos <paulo@matos-sorge.com> 2012-12-20 17:06:04 UTC ---
As per previous comments, I looks at build_one_cst and implemented
build_minus_one_cst:
tree
build_minus_one_cst (tree type)
{
  switch (TREE_CODE (type))
    {
    case INTEGER_TYPE: case ENUMERAL_TYPE: case BOOLEAN_TYPE:
    case POINTER_TYPE: case REFERENCE_TYPE:
    case OFFSET_TYPE:
      return build_int_cst (type, -1);

    case REAL_TYPE:
      return build_real (type, dconstm1);

    case VECTOR_TYPE:
      {
    tree scalar = build_minus_one_cst (TREE_TYPE (type));

    return build_vector_from_val (type, scalar);
      }

    case COMPLEX_TYPE:
      return build_complex (type,
                build_minus_one_cst (TREE_TYPE (type)),
                build_zero_cst (TREE_TYPE (type)));

    case FIXED_POINT_TYPE:
    default:
      gcc_unreachable ();
    }
}


However, I am unsure on how to best model the FIXED_POINT_TYPE case, if at all
possible.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #6 from Paulo J. Matos <paulo@matos-sorge.com> 2013-01-22 15:30:48 UTC ---
I have some further patches that replace the previously posted ones that I will
upload soon. Should these also be sent to gcc-patches or it's unnecessary since
they're being posted here?

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

Paulo J. Matos <paulo@matos-sorge.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29014|0                           |1
        is obsolete|                            |

--- Comment #7 from Paulo J. Matos <paulo@matos-sorge.com> 2013-01-22 16:03:25 UTC ---
Created attachment 29251
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29251
Proposed fix

This is based on Richard's suggestion and it seems to fix the bug.
Should this be submitted to gcc-patches?

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

Paulo J. Matos <paulo@matos-sorge.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29251|0                           |1
        is obsolete|                            |

--- Comment #8 from Paulo J. Matos <paulo@matos-sorge.com> 2013-01-22 16:29:08 UTC ---
Created attachment 29252
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29252
Proprosed fix

Previous patch was removing trailing lines from lines I didn't touch. Now fixed
in this patch.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

Paulo J. Matos <paulo@matos-sorge.com> changed:

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

--- Comment #9 from Paulo J. Matos <paulo@matos-sorge.com> ---
Mark Glisse has submitted the patch for this to HEAD. I guess we can now
comfortably close the report.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #10 from Marc Glisse <glisse at gcc dot gnu.org> ---
Oups, I didn't notice you had already worked on this. Please don't hesitate to
post (and ping) your patch to gcc-patches next time. Also, I didn't touch
tree-tailcall.c, that might still be needed...

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #11 from Paulo J. Matos <paulo@matos-sorge.com> ---
No worries Marc, that's fine. The most important thing is that's fixed. I did
post the patch to patches@ but haven't actually pinged. I tend to forget about
them myself.

Thanks for sorting it out.

[Bug tree-optimization/55761] process_assignment assumes -1 can be created

[paulo@matos-sorge.com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55761

--- Comment #12 from Paulo J. Matos <paulo@matos-sorge.com> ---
Also, I haven't touched tree-tailcall.c on my patches but I can't see why you
would need to do it.