Bug 44507 - [4.5/4.6 Regression] vectorization ANDs array elements together incorrectly
Summary: [4.5/4.6 Regression] vectorization ANDs array elements together incorrectly
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.5.0
: P3 normal
Target Milestone: 4.5.1
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2010-06-12 01:01 UTC by Daniel Davies
Modified: 2010-06-16 03:14 UTC (History)
2 users (show)

See Also:
Host:
Target: i?86-*-*
Build:
Known to work: 4.4.3 4.5.1
Known to fail: 4.5.0
Last reconfirmed: 2010-06-12 08:42:01


Attachments
The preprocessed test case. (3.01 KB, text/plain)
2010-06-12 01:02 UTC, Daniel Davies
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Davies 2010-06-12 01:01:10 UTC
When the enclosed test case is compiled with -O3, the vectorization code loads and SSE register with 0x00000001000000010000000100000001 instead of 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF, then ANDs this with elements of the array.  This causes the code to fail.
Comment 1 Daniel Davies 2010-06-12 01:02:42 UTC
Created attachment 20900 [details]
The preprocessed test case.
Comment 2 Daniel Davies 2010-06-12 01:04:59 UTC
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/4.5.0/lto-wrapper
Target: i386-pc-solaris2.10
Configured with: /tool/gcc/4.5.0/gcc-4.5.0/configure --prefix=/tool/gcc/4.5.0/i386-pc-solaris2.10 --with-gnu-as --with-gnu-ld --disable-shared --enable-lto --with-libelf=/usr/local --enable-gold
Thread model: posix
gcc version 4.5.0 (GCC) 
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=gnu99' '-march=core2' '-pedantic' '-Wall' '-W' '-Wstrict-prototypes' '-Wpointer-arith' '-Wwrite-strings' '-Wcast-qual' '-Wmissing-prototypes' '-fPIC'
 /tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/4.5.0/cc1 -E -quiet -v gcc450VectBug.c -march=core2 -std=gnu99 -pedantic -Wall -W -Wstrict-prototypes -Wpointer-arith -Wwrite-strings -Wcast-qual -Wmissing-prototypes -fPIC -fpch-preprocess -o gcc450VectBug.i
ignoring nonexistent directory "/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../../../i386-pc-solaris2.10/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /tool/gcc/4.5.0/i386-pc-solaris2.10/include
 /tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/include
 /tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/include-fixed
 /usr/include
End of search list.
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=gnu99' '-march=core2' '-pedantic' '-Wall' '-W' '-Wstrict-prototypes' '-Wpointer-arith' '-Wwrite-strings' '-Wcast-qual' '-Wmissing-prototypes' '-fPIC'
 /tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/4.5.0/cc1 -fpreprocessed gcc450VectBug.i -quiet -dumpbase gcc450VectBug.c -march=core2 -auxbase gcc450VectBug -pedantic -Wall -W -Wstrict-prototypes -Wpointer-arith -Wwrite-strings -Wcast-qual -Wmissing-prototypes -std=gnu99 -version -fPIC -o gcc450VectBug.s
GNU C (GCC) version 4.5.0 (i386-pc-solaris2.10)
	compiled by GNU C version 4.5.0, GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
GNU C (GCC) version 4.5.0 (i386-pc-solaris2.10)
	compiled by GNU C version 4.5.0, GMP version 4.3.2, MPFR version 2.4.2, MPC version 0.8.2
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: cf0046f5352e75464cfcf0489db9539c
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=gnu99' '-march=core2' '-pedantic' '-Wall' '-W' '-Wstrict-prototypes' '-Wpointer-arith' '-Wwrite-strings' '-Wcast-qual' '-Wmissing-prototypes' '-fPIC'
 /tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../../../i386-pc-solaris2.10/bin/as -v -V -Qy -s -o gcc450VectBug.o gcc450VectBug.s
GNU assembler version 2.20 (i386-pc-solaris2.10) using BFD version (GNU Binutils) 2.20
COMPILER_PATH=/tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/4.5.0/:/tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/4.5.0/:/tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/:/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/:/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/:/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../../../i386-pc-solaris2.10/bin/:/usr/ccs/bin/
LIBRARY_PATH=/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/:/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../../../i386-pc-solaris2.10/lib/:/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-v' '-save-temps' '-std=gnu99' '-march=core2' '-pedantic' '-Wall' '-W' '-Wstrict-prototypes' '-Wpointer-arith' '-Wwrite-strings' '-Wcast-qual' '-Wmissing-prototypes' '-fPIC'
 /tool/gcc/4.5.0/i386-pc-solaris2.10/libexec/gcc/i386-pc-solaris2.10/4.5.0/collect2 -V -m elf_i386 -Y P,/usr/ccs/lib:/usr/lib -Qy /usr/lib/crt1.o /usr/lib/crti.o /usr/lib/values-Xa.o /tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/crtbegin.o -L/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0 -L/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../../../i386-pc-solaris2.10/lib -L/tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/../../.. gcc450VectBug.o -lgcc -lc -lgcc /tool/gcc/4.5.0/i386-pc-solaris2.10/lib/gcc/i386-pc-solaris2.10/4.5.0/crtend.o /usr/lib/crtn.o
GNU ld (GNU Binutils) 2.20
  Supported emulations:
   elf_i386_ldso
   elf_i386
   elf_x86_64
   elf_l1om
Comment 3 Richard Biener 2010-06-12 08:42:01 UTC
Confirmed.

The vectorizer changes

  # andVal_34 = PHI <andVal_15(5), 0x0ffffffff(3)>
  andVal_15 = curVal_13 & andVal_34;

to

  # vect_var_.26_52 = PHI <vect_var_.26_53(5), { 1, 1, 1, 1 }(3)>
  vect_var_.26_53 = vect_var_.21_42 & vect_var_.26_52;

4.4 works.
Comment 4 H.J. Lu 2010-06-12 15:52:11 UTC
It is caused by revision 149526:

http://gcc.gnu.org/ml/gcc-cvs/2009-07/msg00406.html
Comment 5 Ira Rosen 2010-06-13 10:29:36 UTC
The bug is in creation of a neutral value for BIT_AND_EXPR. What is the correct way to create it for all types? I found 
double-int.h:#define ALL_ONES (~((unsigned HOST_WIDE_INT) 0))
but it won't work for signed.

Thanks,
Ira
Comment 6 Richard Biener 2010-06-13 11:35:06 UTC
(In reply to comment #5)
> The bug is in creation of a neutral value for BIT_AND_EXPR. What is the correct
> way to create it for all types? I found 
> double-int.h:#define ALL_ONES (~((unsigned HOST_WIDE_INT) 0))
> but it won't work for signed.

  build_int_cst (type, -1)

At least in tree-vect-slp.c:1669 this seems to be buggy.  The
case for BIT_AND_EXPR should be separated from that of MULT_EXPR.

> Thanks,
> Ira
> 

Comment 7 Ira Rosen 2010-06-13 12:01:48 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > The bug is in creation of a neutral value for BIT_AND_EXPR. What is the correct
> > way to create it for all types? I found 
> > double-int.h:#define ALL_ONES (~((unsigned HOST_WIDE_INT) 0))
> > but it won't work for signed.
> 
>   build_int_cst (type, -1)

OK, thanks.

> 
> At least in tree-vect-slp.c:1669 this seems to be buggy.  The
> case for BIT_AND_EXPR should be separated from that of MULT_EXPR.

Right, this is buggy too, but the failure here is in reduction (get_initial_def_for_reduction), not in SLP.

Is it safe to assume that operands of BIT_AND_EXPR are of integral type? If so, I'll test the following patch:

Index: tree-vect-loop.c
===================================================================
--- tree-vect-loop.c    (revision 160524)
+++ tree-vect-loop.c    (working copy)
@@ -2871,12 +2871,15 @@ get_initial_def_for_reduction (gimple st
               *adjustment_def = init_val;
           }

-        if (code == MULT_EXPR || code == BIT_AND_EXPR)
+        if (code == MULT_EXPR)
           {
             real_init_val = dconst1;
             int_init_val = 1;
           }

+        if (code == BIT_AND_EXPR)
+          int_init_val = -1;
+
         if (SCALAR_FLOAT_TYPE_P (scalar_type))
           def_for_init = build_real (scalar_type, real_init_val);
         else
Index: tree-vect-slp.c
===================================================================
--- tree-vect-slp.c     (revision 160524)
+++ tree-vect-slp.c     (working copy)
@@ -1662,7 +1662,6 @@ vect_get_constant_vectors (slp_tree slp_
              break;

           case MULT_EXPR:
-          case BIT_AND_EXPR:
              if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (op)))
                neutral_op = build_real (TREE_TYPE (op), dconst1);
              else
@@ -1670,6 +1669,10 @@ vect_get_constant_vectors (slp_tree slp_

              break;

+          case BIT_AND_EXPR:
+            neutral_op = build_int_cst (TREE_TYPE (op), -1);
+            break;
+
           default:
              neutral_op = NULL;
         }

Thanks,
Ira
Comment 8 Richard Biener 2010-06-13 12:25:35 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > The bug is in creation of a neutral value for BIT_AND_EXPR. What is the correct
> > > way to create it for all types? I found 
> > > double-int.h:#define ALL_ONES (~((unsigned HOST_WIDE_INT) 0))
> > > but it won't work for signed.
> > 
> >   build_int_cst (type, -1)
> 
> OK, thanks.
> 
> > 
> > At least in tree-vect-slp.c:1669 this seems to be buggy.  The
> > case for BIT_AND_EXPR should be separated from that of MULT_EXPR.
> 
> Right, this is buggy too, but the failure here is in reduction
> (get_initial_def_for_reduction), not in SLP.
> 
> Is it safe to assume that operands of BIT_AND_EXPR are of integral type?

Yes, I think so (vector types are ok as well, but we don't vectorize with
vector types around, maybe fixed-point types are ok as well, but we
have no vector modes for them)

> If so,
> I'll test the following patch:
> 
> Index: tree-vect-loop.c
> ===================================================================
> --- tree-vect-loop.c    (revision 160524)
> +++ tree-vect-loop.c    (working copy)
> @@ -2871,12 +2871,15 @@ get_initial_def_for_reduction (gimple st
>                *adjustment_def = init_val;
>            }
> 
> -        if (code == MULT_EXPR || code == BIT_AND_EXPR)
> +        if (code == MULT_EXPR)
>            {
>              real_init_val = dconst1;
>              int_init_val = 1;
>            }
> 
> +        if (code == BIT_AND_EXPR)
> +          int_init_val = -1;
> +
>          if (SCALAR_FLOAT_TYPE_P (scalar_type))
>            def_for_init = build_real (scalar_type, real_init_val);
>          else
> Index: tree-vect-slp.c
> ===================================================================
> --- tree-vect-slp.c     (revision 160524)
> +++ tree-vect-slp.c     (working copy)
> @@ -1662,7 +1662,6 @@ vect_get_constant_vectors (slp_tree slp_
>               break;
> 
>            case MULT_EXPR:
> -          case BIT_AND_EXPR:
>               if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (op)))
>                 neutral_op = build_real (TREE_TYPE (op), dconst1);
>               else
> @@ -1670,6 +1669,10 @@ vect_get_constant_vectors (slp_tree slp_
> 
>               break;
> 
> +          case BIT_AND_EXPR:
> +            neutral_op = build_int_cst (TREE_TYPE (op), -1);
> +            break;
> +
>            default:
>               neutral_op = NULL;
>          }

Looks good.

Thanks,
Richard.

> Thanks,
> Ira
> 

Comment 9 irar 2010-06-14 12:22:44 UTC
Subject: Bug 44507

Author: irar
Date: Mon Jun 14 12:22:13 2010
New Revision: 160727

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160727
Log:

	PR tree-optimization/44507 
	* tree-vect-loop.c (get_initial_def_for_reduction): Use -1
	to build initial vector for BIT_AND_EXPR.
	* tree-vect-slp.c (vect_get_constant_vectors): Likewise.


Added:
    trunk/gcc/testsuite/gcc.dg/vect/pr44507.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vect-loop.c
    trunk/gcc/tree-vect-slp.c

Comment 10 irar 2010-06-14 13:43:42 UTC
Subject: Bug 44507

Author: irar
Date: Mon Jun 14 13:43:24 2010
New Revision: 160742

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160742
Log:

	PR tree-optimization/44507
	* tree-vect-loop.c (get_initial_def_for_reduction): Use -1
	to build initial vector for BIT_AND_EXPR.


Added:
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/pr44507.c
Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_5-branch/gcc/tree-vect-loop.c

Comment 11 Richard Biener 2010-06-14 19:03:02 UTC
Fixed.
Comment 12 Alan Modra 2010-06-16 03:14:12 UTC
testsuite/gcc.dg/vect/pr44507.c is invalid on LP64.  This:

    curVal = *((unsigned long *)(&pArray[index]));

loads 8 bytes, ie. the last time around the loop this loads 4 bytes past the end of the array.  On big-endian machines this is the low 32 bits so the test fails.  The obvious fix is to replace the cast with (unsigned int *) and indeed this makes the test pass on powerpc64.
Comment 13 Alan Modra 2010-06-16 04:22:55 UTC
Subject: Bug 44507

Author: amodra
Date: Wed Jun 16 04:22:36 2010
New Revision: 160821

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160821
Log:
	PR tree-optimization/44507
	* gcc.dg/vect/pr44507.c (seeIf256ByteArrayIsConstant): Correct cast.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/vect/pr44507.c

Comment 14 Alan Modra 2010-06-16 04:23:33 UTC
Subject: Bug 44507

Author: amodra
Date: Wed Jun 16 04:23:05 2010
New Revision: 160822

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160822
Log:
	PR tree-optimization/44507
	* gcc.dg/vect/pr44507.c (seeIf256ByteArrayIsConstant): Correct cast.


Modified:
    branches/gcc-4_5-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_5-branch/gcc/testsuite/gcc.dg/vect/pr44507.c