diff mbox

PR 58958: wrong aliasing info

Message ID alpine.DEB.2.02.1311051132590.30766@stedding.saclay.inria.fr
State New
Headers show

Commit Message

Marc Glisse Nov. 5, 2013, 10:40 a.m. UTC
On Mon, 4 Nov 2013, Richard Biener wrote:

> Well, you cannot use the size argument unchanged for the null return 
> case.  You could fallback to get_base_address and -1 size in that case.

Like this? Bootstrap+testsuite on x86_64-unknown-linux-gnu.
(I think I'll disable cilk for my future bootstraps: it takes forever and 
confuses contrib/compare_tests)

2013-11-05  Marc Glisse  <marc.glisse@inria.fr>

 	PR tree-optimization/58958
gcc/
         * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Use
 	get_addr_base_and_unit_offset instead of get_ref_base_and_extent.

gcc/testsuite/
 	* gcc.dg/tree-ssa/pr58958.c: New file.

Comments

Jakub Jelinek Nov. 5, 2013, 11:02 a.m. UTC | #1
On Tue, Nov 05, 2013 at 11:40:02AM +0100, Marc Glisse wrote:
> >Well, you cannot use the size argument unchanged for the null
> >return case.  You could fallback to get_base_address and -1 size
> >in that case.
> 
> Like this? Bootstrap+testsuite on x86_64-unknown-linux-gnu.
> (I think I'll disable cilk for my future bootstraps: it takes
> forever and confuses contrib/compare_tests)

Ah, I was wondering why my make check times (admittedly
--enable-checking=yes,rtl but I've done that for years) went up drastically
between Friday and Monday (from around 40 minutes to 70 minutes or more).

That is clearly highly undesirable.  Looking at
gcc.dg/cilk-plus/cilk-plus.exp
it seems every test is run 24 resp. 25 times, that is clearly way too much,
sure, we have torture kind of tests, but that is typically 6 or 8 times at
most, and it also depends on how expensive the tests are.
As the /AN/ tests were already preexisting, I guess we are talking about
the 9 new /CK/ dg-do run tests, so first of all, they must be very expensive
themselves, times 25:

dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus $ALWAYS_CFLAGS " " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O0 -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O1 -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O2 -ftree-vectorize -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O3 -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -g -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -g -O0 -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -g -O1 -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -g -O2 -ftree-vectorize -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -g -O3 -fcilkplus $ALWAYS_CFLAGS"  " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O3 -ftree-vectorize -fcilkplus -g $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -O0 -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -O1 -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -O2 -ftree-vectorize -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -O3 -std=c99 $ALWAYS_CFLAGS"  " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -g -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -g -O0 -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -g -O1 -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -g -O2 -ftree-vectorize -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -fcilkplus -g -O3 -std=c99 $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O3 -ftree-vectorize -std=c99 -g -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O0 -flto -g -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O2 -flto -g -fcilkplus $ALWAYS_CFLAGS" " "
dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]] " -O3  -flto -g -fcilkplus $ALWAYS_CFLAGS" " "

It doesn't make sense to test -g at all levels, just test it for one or two, it doesn't
make sense to test say no -O* vs. -O0, that is the same thing, or -O3 vs. -O3 -ftree-vectorize,
that is the same thing, testing -std=c99 vs. default is reasonable for compile time tests
if they particularly care for some reason, otherwise, just attach -std=c99 to one (doesn't matter
which one) variant.  So, please change this to at most 7 or so variants, and even then, if some of
the test is really expensive, use it as dg-do compile for all variants and dg-do run just for
one (say -O2 -ftree-vectorize or -O3 -g).

	Jakub
Richard Biener Nov. 5, 2013, 12:05 p.m. UTC | #2
On Tue, Nov 5, 2013 at 11:40 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 4 Nov 2013, Richard Biener wrote:
>
>> Well, you cannot use the size argument unchanged for the null return case.
>> You could fallback to get_base_address and -1 size in that case.
>
>
> Like this? Bootstrap+testsuite on x86_64-unknown-linux-gnu.
> (I think I'll disable cilk for my future bootstraps: it takes forever and
> confuses contrib/compare_tests)

Yes, this is ok.

Thanks,
Richard.

> 2013-11-05  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR tree-optimization/58958
> gcc/
>         * tree-ssa-alias.c (ao_ref_init_from_ptr_and_size): Use
>         get_addr_base_and_unit_offset instead of get_ref_base_and_extent.
>
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/pr58958.c: New file.
>
> --
> Marc Glisse
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr58958.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr58958.c     (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr58958.c     (working copy)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +double a[10];
> +int f(int n){
> +  a[3]=9;
> +  __builtin_memset(&a[n],3,sizeof(double));
> +  return a[3]==9;
> +}
> +
> +/* { dg-final { scan-tree-dump " == 9" "optimized" } } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
>
> Property changes on: gcc/testsuite/gcc.dg/tree-ssa/pr58958.c
> ___________________________________________________________________
> Added: svn:keywords
> ## -0,0 +1 ##
> +Author Date Id Revision URL
> \ No newline at end of property
> Added: svn:eol-style
> ## -0,0 +1 ##
> +native
> \ No newline at end of property
> Index: gcc/tree-ssa-alias.c
> ===================================================================
> --- gcc/tree-ssa-alias.c        (revision 204381)
> +++ gcc/tree-ssa-alias.c        (working copy)
> @@ -559,41 +559,50 @@ ao_ref_alias_set (ao_ref *ref)
>  }
>
>  /* Init an alias-oracle reference representation from a gimple pointer
>     PTR and a gimple size SIZE in bytes.  If SIZE is NULL_TREE the the
>     size is assumed to be unknown.  The access is assumed to be only
>     to or after of the pointer target, not before it.  */
>
>  void
>  ao_ref_init_from_ptr_and_size (ao_ref *ref, tree ptr, tree size)
>  {
> -  HOST_WIDE_INT t1, t2, extra_offset = 0;
> +  HOST_WIDE_INT t, extra_offset = 0;
>    ref->ref = NULL_TREE;
>    if (TREE_CODE (ptr) == SSA_NAME)
>      {
>        gimple stmt = SSA_NAME_DEF_STMT (ptr);
>        if (gimple_assign_single_p (stmt)
>           && gimple_assign_rhs_code (stmt) == ADDR_EXPR)
>         ptr = gimple_assign_rhs1 (stmt);
>        else if (is_gimple_assign (stmt)
>                && gimple_assign_rhs_code (stmt) == POINTER_PLUS_EXPR
>                && host_integerp (gimple_assign_rhs2 (stmt), 0)
> -              && (t1 = int_cst_value (gimple_assign_rhs2 (stmt))) >= 0)
> +              && (t = int_cst_value (gimple_assign_rhs2 (stmt))) >= 0)
>         {
>           ptr = gimple_assign_rhs1 (stmt);
> -         extra_offset = BITS_PER_UNIT * t1;
> +         extra_offset = BITS_PER_UNIT * t;
>         }
>      }
>
>    if (TREE_CODE (ptr) == ADDR_EXPR)
> -    ref->base = get_ref_base_and_extent (TREE_OPERAND (ptr, 0),
> -                                        &ref->offset, &t1, &t2);
> +    {
> +      ref->base = get_addr_base_and_unit_offset (TREE_OPERAND (ptr, 0),
> &t);
> +      if (ref->base)
> +       ref->offset = BITS_PER_UNIT * t;
> +      else
> +       {
> +         size = NULL_TREE;
> +         ref->offset = 0;
> +         ref->base = get_base_address (TREE_OPERAND (ptr, 0));
> +       }
> +    }
>    else
>      {
>        ref->base = build2 (MEM_REF, char_type_node,
>                           ptr, null_pointer_node);
>        ref->offset = 0;
>      }
>    ref->offset += extra_offset;
>    if (size
>        && host_integerp (size, 0)
>        && TREE_INT_CST_LOW (size) * BITS_PER_UNIT / BITS_PER_UNIT
>
Iyer, Balaji V Nov. 5, 2013, 1:55 p.m. UTC | #3
> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Tuesday, November 5, 2013 6:02 AM
> To: Marc Glisse; Iyer, Balaji V
> Cc: Richard Biener; GCC Patches
> Subject: Re: PR 58958: wrong aliasing info
> 
> On Tue, Nov 05, 2013 at 11:40:02AM +0100, Marc Glisse wrote:
> > >Well, you cannot use the size argument unchanged for the null return
> > >case.  You could fallback to get_base_address and -1 size in that
> > >case.
> >
> > Like this? Bootstrap+testsuite on x86_64-unknown-linux-gnu.
> > (I think I'll disable cilk for my future bootstraps: it takes forever
> > and confuses contrib/compare_tests)
> 
> Ah, I was wondering why my make check times (admittedly --enable-
> checking=yes,rtl but I've done that for years) went up drastically between
> Friday and Monday (from around 40 minutes to 70 minutes or more).
> 
> That is clearly highly undesirable.  Looking at gcc.dg/cilk-plus/cilk-plus.exp it
> seems every test is run 24 resp. 25 times, that is clearly way too much, sure,
> we have torture kind of tests, but that is typically 6 or 8 times at most, and it
> also depends on how expensive the tests are.
> As the /AN/ tests were already preexisting, I guess we are talking about the 9
> new /CK/ dg-do run tests, so first of all, they must be very expensive
> themselves, times 25:
> 
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus $ALWAYS_CFLAGS " " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O0 -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O1 -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O2 -ftree-vectorize -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O3 -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -g -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -g -O0 -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -g -O1 -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -g -O2 -ftree-vectorize -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -g -O3 -fcilkplus $ALWAYS_CFLAGS"  " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O3 -ftree-vectorize -fcilkplus -g $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -O0 -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -O1 -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -O2 -ftree-vectorize -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -O3 -std=c99 $ALWAYS_CFLAGS"  " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -g -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -g -O0 -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -g -O1 -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -g -O2 -ftree-vectorize -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -fcilkplus -g -O3 -std=c99 $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O3 -ftree-vectorize -std=c99 -g -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O0 -flto -g -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O2 -flto -g -fcilkplus $ALWAYS_CFLAGS" " "
> dg-runtest [lsort [glob -nocomplain $srcdir/c-c++-common/cilk-plus/CK/*.c]]
> " -O3  -flto -g -fcilkplus $ALWAYS_CFLAGS" " "
> 
> It doesn't make sense to test -g at all levels, just test it for one or two, it
> doesn't make sense to test say no -O* vs. -O0, that is the same thing, or -O3
> vs. -O3 -ftree-vectorize, that is the same thing, testing -std=c99 vs. default is
> reasonable for compile time tests if they particularly care for some reason,
> otherwise, just attach -std=c99 to one (doesn't matter which one) variant.
> So, please change this to at most 7 or so variants, and even then, if some of
> the test is really expensive, use it as dg-do compile for all variants and dg-do
> run just for one (say -O2 -ftree-vectorize or -O3 -g).
> 

Hi Jakub,
    Let me go through the testsuite script and I will remove some of the duplicates and send you a patch ASAP.

Thanks,

Balaji V. Iyer.

> 	Jakub
diff mbox

Patch

Index: gcc/testsuite/gcc.dg/tree-ssa/pr58958.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr58958.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr58958.c	(working copy)
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+double a[10];
+int f(int n){
+  a[3]=9;
+  __builtin_memset(&a[n],3,sizeof(double));
+  return a[3]==9;
+}
+
+/* { dg-final { scan-tree-dump " == 9" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */

Property changes on: gcc/testsuite/gcc.dg/tree-ssa/pr58958.c
___________________________________________________________________
Added: svn:keywords
## -0,0 +1 ##
+Author Date Id Revision URL
\ No newline at end of property
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 204381)
+++ gcc/tree-ssa-alias.c	(working copy)
@@ -559,41 +559,50 @@  ao_ref_alias_set (ao_ref *ref)
 }
 
 /* Init an alias-oracle reference representation from a gimple pointer
    PTR and a gimple size SIZE in bytes.  If SIZE is NULL_TREE the the
    size is assumed to be unknown.  The access is assumed to be only
    to or after of the pointer target, not before it.  */
 
 void
 ao_ref_init_from_ptr_and_size (ao_ref *ref, tree ptr, tree size)
 {
-  HOST_WIDE_INT t1, t2, extra_offset = 0;
+  HOST_WIDE_INT t, extra_offset = 0;
   ref->ref = NULL_TREE;
   if (TREE_CODE (ptr) == SSA_NAME)
     {
       gimple stmt = SSA_NAME_DEF_STMT (ptr);
       if (gimple_assign_single_p (stmt)
 	  && gimple_assign_rhs_code (stmt) == ADDR_EXPR)
 	ptr = gimple_assign_rhs1 (stmt);
       else if (is_gimple_assign (stmt)
 	       && gimple_assign_rhs_code (stmt) == POINTER_PLUS_EXPR
 	       && host_integerp (gimple_assign_rhs2 (stmt), 0)
-	       && (t1 = int_cst_value (gimple_assign_rhs2 (stmt))) >= 0)
+	       && (t = int_cst_value (gimple_assign_rhs2 (stmt))) >= 0)
 	{
 	  ptr = gimple_assign_rhs1 (stmt);
-	  extra_offset = BITS_PER_UNIT * t1;
+	  extra_offset = BITS_PER_UNIT * t;
 	}
     }
 
   if (TREE_CODE (ptr) == ADDR_EXPR)
-    ref->base = get_ref_base_and_extent (TREE_OPERAND (ptr, 0),
-					 &ref->offset, &t1, &t2);
+    {
+      ref->base = get_addr_base_and_unit_offset (TREE_OPERAND (ptr, 0), &t);
+      if (ref->base)
+	ref->offset = BITS_PER_UNIT * t;
+      else
+	{
+	  size = NULL_TREE;
+	  ref->offset = 0;
+	  ref->base = get_base_address (TREE_OPERAND (ptr, 0));
+	}
+    }
   else
     {
       ref->base = build2 (MEM_REF, char_type_node,
 			  ptr, null_pointer_node);
       ref->offset = 0;
     }
   ref->offset += extra_offset;
   if (size
       && host_integerp (size, 0)
       && TREE_INT_CST_LOW (size) * BITS_PER_UNIT / BITS_PER_UNIT