Patchwork Fix vectorizable_store

login
register
mail settings
Submitter Jakub Jelinek
Date June 26, 2013, 7:53 p.m.
Message ID <20130626195345.GY2336@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/254851/
State New
Headers show

Comments

Jakub Jelinek - June 26, 2013, 7:53 p.m.
Hi!

On the gomp4 branch I've noticed a miscompilation of the simd-3.C
testcase I'm adding there, but even say
int a[1024] __attribute__((aligned (32))) = { 1 };
int b[1024] __attribute__((aligned (32))) = { 1 };
unsigned short c[1024] __attribute__((aligned (32))) = { 1 };

__attribute__((noinline, noclone)) void
foo (int *p)
{
  int i;
  p = (int *) __builtin_assume_aligned (p, 32);
  for (i = 0; i < 512; i++)
    {
      a[i] *= p[i];
      c[i]++;
    }
}

int
main ()
{
  int i;
  for (i = 0; i < 512; i++)
    {
      a[i] = i - 512;
      b[i] = (i - 51) % 39;
      c[i] = (unsigned short) i;
    }
  foo (b);
  for (i = 0; i < 512; i++)
    if (b[i] != (i - 51) % 39
	|| a[i] != (i - 512) * b[i]
	|| c[i] != (unsigned short) (i + 1))
      __builtin_abort ();
  return 0;
}
without -fopenmp, just -O3 -mavx.  The relevant change was just
that ptr_incr has been initialized to NULL in both vectorizable_store
and vectorizable_load, because vect_create_data_ref_ptr doesn't initialize
it if only_init is true.  Looking at it, we just trigger undefined behavior
and are just lucky that it works by accident on the trunk and branches.
The problem is that if ncopies > 1, vectorizable_store declares the variable
inside of the loop, for j == 0 it initializes it through
vect_create_data_ref_ptr, but then on the next iteration the variable is
uninitialized again (but just due to luck contains the value from the
previous iteration, but say if compiler unrolled the loop, it would already
misbehave) and then it is passed to bump_vector_ptr.  Fixed by moving the
var decl outside of the loop.  While not strictly necessary, I find it
cleaner to initialize it to NULL, though if you disagree with it, I can keep
that change local to gomp4 branch for now (i.e. remove " = NULL" from the
first hunk and the third hunk).

Ok for trunk/4.8?

2013-06-26  Jakub Jelinek  <jakub@redhat.com>

	* tree-vect-stmts.c (vectorizable_store): Move ptr_incr var
	decl before the loop, initialize to NULL.
	(vectorizable_load): Initialize ptr_incr to NULL.



	Jakub
Richard Guenther - June 27, 2013, 3:30 p.m.
Jakub Jelinek <jakub@redhat.com> wrote:

>Hi!
>
>On the gomp4 branch I've noticed a miscompilation of the simd-3.C
>testcase I'm adding there, but even say
>int a[1024] __attribute__((aligned (32))) = { 1 };
>int b[1024] __attribute__((aligned (32))) = { 1 };
>unsigned short c[1024] __attribute__((aligned (32))) = { 1 };
>
>__attribute__((noinline, noclone)) void
>foo (int *p)
>{
>  int i;
>  p = (int *) __builtin_assume_aligned (p, 32);
>  for (i = 0; i < 512; i++)
>    {
>      a[i] *= p[i];
>      c[i]++;
>    }
>}
>
>int
>main ()
>{
>  int i;
>  for (i = 0; i < 512; i++)
>    {
>      a[i] = i - 512;
>      b[i] = (i - 51) % 39;
>      c[i] = (unsigned short) i;
>    }
>  foo (b);
>  for (i = 0; i < 512; i++)
>    if (b[i] != (i - 51) % 39
>	|| a[i] != (i - 512) * b[i]
>	|| c[i] != (unsigned short) (i + 1))
>      __builtin_abort ();
>  return 0;
>}
>without -fopenmp, just -O3 -mavx.  The relevant change was just
>that ptr_incr has been initialized to NULL in both vectorizable_store
>and vectorizable_load, because vect_create_data_ref_ptr doesn't
>initialize
>it if only_init is true.  Looking at it, we just trigger undefined
>behavior
>and are just lucky that it works by accident on the trunk and branches.
>The problem is that if ncopies > 1, vectorizable_store declares the
>variable
>inside of the loop, for j == 0 it initializes it through
>vect_create_data_ref_ptr, but then on the next iteration the variable
>is
>uninitialized again (but just due to luck contains the value from the
>previous iteration, but say if compiler unrolled the loop, it would
>already
>misbehave) and then it is passed to bump_vector_ptr.  Fixed by moving
>the
>var decl outside of the loop.  While not strictly necessary, I find it
>cleaner to initialize it to NULL, though if you disagree with it, I can
>keep
>that change local to gomp4 branch for now (i.e. remove " = NULL" from
>the
>first hunk and the third hunk).
>
>Ok for trunk/4.8?
>
Ok

Thanks,
Richard.
>2013-06-26  Jakub Jelinek  <jakub@redhat.com>
>
>	* tree-vect-stmts.c (vectorizable_store): Move ptr_incr var
>	decl before the loop, initialize to NULL.
>	(vectorizable_load): Initialize ptr_incr to NULL.
>
>--- gcc/tree-vect-stmts.c.jj	2013-04-22 08:06:41.000000000 +0200
>+++ gcc/tree-vect-stmts.c	2013-06-26 21:34:28.609654773 +0200
>@@ -3796,6 +3796,7 @@ vectorizable_store (gimple stmt, gimple_
>   enum vect_def_type dt;
>   stmt_vec_info prev_stmt_info = NULL;
>   tree dataref_ptr = NULL_TREE;
>+  gimple ptr_incr = NULL;
>   int nunits = TYPE_VECTOR_SUBPARTS (vectype);
>   int ncopies;
>   int j;
>@@ -4041,7 +4042,6 @@ vectorizable_store (gimple stmt, gimple_
>   for (j = 0; j < ncopies; j++)
>     {
>       gimple new_stmt;
>-      gimple ptr_incr;
> 
>       if (j == 0)
> 	{
>@@ -4314,7 +4314,7 @@ vectorizable_load (gimple stmt, gimple_s
>   tree dummy;
>   enum dr_alignment_support alignment_support_scheme;
>   tree dataref_ptr = NULL_TREE;
>-  gimple ptr_incr;
>+  gimple ptr_incr = NULL;
>   int nunits = TYPE_VECTOR_SUBPARTS (vectype);
>   int ncopies;
>   int i, j, group_size, group_gap;
>
>
>	Jakub

Patch

--- gcc/tree-vect-stmts.c.jj	2013-04-22 08:06:41.000000000 +0200
+++ gcc/tree-vect-stmts.c	2013-06-26 21:34:28.609654773 +0200
@@ -3796,6 +3796,7 @@  vectorizable_store (gimple stmt, gimple_
   enum vect_def_type dt;
   stmt_vec_info prev_stmt_info = NULL;
   tree dataref_ptr = NULL_TREE;
+  gimple ptr_incr = NULL;
   int nunits = TYPE_VECTOR_SUBPARTS (vectype);
   int ncopies;
   int j;
@@ -4041,7 +4042,6 @@  vectorizable_store (gimple stmt, gimple_
   for (j = 0; j < ncopies; j++)
     {
       gimple new_stmt;
-      gimple ptr_incr;
 
       if (j == 0)
 	{
@@ -4314,7 +4314,7 @@  vectorizable_load (gimple stmt, gimple_s
   tree dummy;
   enum dr_alignment_support alignment_support_scheme;
   tree dataref_ptr = NULL_TREE;
-  gimple ptr_incr;
+  gimple ptr_incr = NULL;
   int nunits = TYPE_VECTOR_SUBPARTS (vectype);
   int ncopies;
   int i, j, group_size, group_gap;