Patchwork Improve VPR for some builtins and non pointer checks

login
register
mail settings
Submitter Andrew Pinski
Date Sept. 3, 2012, 5:18 a.m.
Message ID <CA+=Sn1merkL4qBDb-WO_OAgfSNyXYsq42t1yUBSRd7qY2oP_=w@mail.gmail.com>
Download mbox | patch
Permalink /patch/181295/
State New
Headers show

Comments

Andrew Pinski - Sept. 3, 2012, 5:18 a.m.
Hi,
  While fixing some code not to have aliasing violations in it, I can
across that some builtins were not causing their arguments or their
return values being marked as non-null.  This patch implements just
that in VPR while allowing to remove some null pointer checks later
on.

OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

ChangeLog:
        * tree-vrp.c (vrp_stmt_computes_nonzero): Return true for some
        builtins (memcpy and memmove).
        (infer_value_range): Infer nonzero for some arguments to
        some builtins (memcpy, memmove, strcmp and memcmp).

testsuite/ChangeLog:
        * gcc.dg/tree-ssa/vrp-builtins1.c: New testcase.
Georg-Johann Lay - Sept. 3, 2012, 6:36 a.m.
Andrew Pinski schrieb:
> Hi,
>   While fixing some code not to have aliasing violations in it, I can
> across that some builtins were not causing their arguments or their
> return values being marked as non-null.  This patch implements just
> that in VPR while allowing to remove some null pointer checks later
> on.
> 
> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> 
> Thanks,
> Andrew Pinski
> 
> ChangeLog:
>         * tree-vrp.c (vrp_stmt_computes_nonzero): Return true for some
>         builtins (memcpy and memmove).
>         (infer_value_range): Infer nonzero for some arguments to
>         some builtins (memcpy, memmove, strcmp and memcmp).
> 
> testsuite/ChangeLog:
>         * gcc.dg/tree-ssa/vrp-builtins1.c: New testcase.
> 
> 
> 
> Index: tree-vrp.c
> ===================================================================
> --- tree-vrp.c	(revision 190868)
> +++ tree-vrp.c	(working copy)
> @@ -1057,6 +1057,20 @@ vrp_stmt_computes_nonzero (gimple stmt,
>  	}
>      }
>  
> +  /* With some builtins, we can infer if the pointer return value
> +     will be non null.  */
> +  if (flag_delete_null_pointer_checks
> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt)
> +      && DECL_BUILT_IN_CLASS (gimple_call_fndecl (stmt)) == BUILT_IN_NORMAL)
> +    {
> +      switch (DECL_FUNCTION_CODE (gimple_call_fndecl (stmt)))
> +	{
> +	  case BUILT_IN_MEMCPY:
> +	  case BUILT_IN_MEMMOVE:
> +	    return true;
> +	}
> +    }
> +
>    return false;
>  }
>  
> @@ -4231,6 +4245,32 @@ infer_value_range (gimple stmt, tree op,
>  	}
>      }
>  
> +  /* With some builtins, we can infer if the pointer argument
> +     will be non null.  */
> +  if (flag_delete_null_pointer_checks
> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt))
> +    {
> +      tree callee = gimple_call_fndecl (stmt);
> +      if (DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
> +	{
> +	  switch (DECL_FUNCTION_CODE (callee))
> +	    {
> +	      case BUILT_IN_MEMCPY:
> +	      case BUILT_IN_MEMMOVE:
> +	      case BUILT_IN_STRCMP:
> +	      case BUILT_IN_MEMCMP:
> +		/* The first and second arguments of memcpy and memmove will be non null after the call. */
> +		if (gimple_call_arg (stmt, 0) == op
> +		    || gimple_call_arg (stmt, 1) == op)
> +		  {
> +		    *val_p = build_int_cst (TREE_TYPE (op), 0);
> +		    *comp_code_p = NE_EXPR;
> +		    return true;
> +		  }
> +	    }
> +	}
> +    }
> +
>    return false;
>  }
>  
> Index: testsuite/gcc.dg/tree-ssa/vrp-builtins1.c
> ===================================================================
> --- testsuite/gcc.dg/tree-ssa/vrp-builtins1.c	(revision 0)
> +++ testsuite/gcc.dg/tree-ssa/vrp-builtins1.c	(revision 0)
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-vrp1" } */

Shouldn't this xfail for avr and other targets that set
flag_delete_null_pointer_checks = 0, cf.
avr.c:avr_option_override() for example.

Johann

> +
> +struct f1
> +{
> +char a[4];
> +};
> +
> +int f(int *a, struct f1 b)
> +{
> +  int *c = __builtin_memcpy(a, b.a, 4);
> +  if (c == 0)
> +    return 0;
> +  return *a;
> +}
> +
> +
> +int f1(int *a, struct f1 b)
> +{
> +  int *c = __builtin_memcpy(a, b.a, 4);
> +  if (a == 0)
> +    return 0;
> +  return *a;
> +}
> +
> +/* Both the if statements should be folded when the target does not keep around null pointer checks. */
> +/* { dg-final { scan-tree-dump-times "Folding predicate" 0 "vrp1" { target {   keeps_null_pointer_checks } } } } */
> +/* { dg-final { scan-tree-dump-times "Folding predicate" 2 "vrp1" { target { ! keeps_null_pointer_checks } } } } */
> +/* { dg-final { cleanup-tree-dump "vrp1" } } */
> +
Jakub Jelinek - Sept. 3, 2012, 7:03 a.m.
On Sun, Sep 02, 2012 at 10:18:15PM -0700, Andrew Pinski wrote:
>   While fixing some code not to have aliasing violations in it, I can
> across that some builtins were not causing their arguments or their
> return values being marked as non-null.  This patch implements just
> that in VPR while allowing to remove some null pointer checks later
> on.
> 
> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> @@ -1057,6 +1057,20 @@ vrp_stmt_computes_nonzero (gimple stmt,
>  	}
>      }
>  
> +  /* With some builtins, we can infer if the pointer return value
> +     will be non null.  */
> +  if (flag_delete_null_pointer_checks
> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt)
> +      && DECL_BUILT_IN_CLASS (gimple_call_fndecl (stmt)) == BUILT_IN_NORMAL)
> +    {
> +      switch (DECL_FUNCTION_CODE (gimple_call_fndecl (stmt)))
> +	{
> +	  case BUILT_IN_MEMCPY:
> +	  case BUILT_IN_MEMMOVE:
> +	    return true;
> +	}
> +    }
> +
>    return false;
>  }
>  

That is too hackish and lists way too few builtins.
If you rely on nonnull attribute marked builtins, I'd say you want
flags = gimple_call_return_flags (stmt);
if ((flags & ERF_RETURNS_ARG)
    && (flags & ERF_RETURN_ARG_MASK) < gimple_call_num_args (stmt))
  {
    /* Test nonnull attribute on the decl, either argument-less or
       on the (flags & ERF_RETURN_ARG_MASK)th argument.  */
  }
Or at least handle builtins e.g. CCP handles as pass-thru arg1:
BUILT_IN_MEMCPY, BUILT_IN_MEMMOVE, BUILT_IN_MEMSET, BUILT_IN_STRCPY,
BUILT_IN_STRNCPY, BUILT_IN_MEMCPY_CHK, BUILT_IN_MEMMOVE_CHK,
BUILT_IN_MEMSET_CHK, BUILT_IN_STRCPY_CHK, BUILT_IN_STRNCPY_CHK
(which reminds me that some of these apparently aren't marked
with ATTR_RET1_NOTHROW_NONNULL_LEAF, why?).

> @@ -4231,6 +4245,32 @@ infer_value_range (gimple stmt, tree op,
>  	}
>      }
>  
> +  /* With some builtins, we can infer if the pointer argument
> +     will be non null.  */
> +  if (flag_delete_null_pointer_checks
> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt))
> +    {
> +      tree callee = gimple_call_fndecl (stmt);
> +      if (DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
> +	{
> +	  switch (DECL_FUNCTION_CODE (callee))
> +	    {
> +	      case BUILT_IN_MEMCPY:
> +	      case BUILT_IN_MEMMOVE:
> +	      case BUILT_IN_STRCMP:
> +	      case BUILT_IN_MEMCMP:
> +		/* The first and second arguments of memcpy and memmove will be non null after the call. */
> +		if (gimple_call_arg (stmt, 0) == op
> +		    || gimple_call_arg (stmt, 1) == op)
> +		  {
> +		    *val_p = build_int_cst (TREE_TYPE (op), 0);
> +		    *comp_code_p = NE_EXPR;
> +		    return true;
> +		  }
> +	    }
> +	}
> +    }

Again, what you are looking for here?  Passing pointers to nonnull
attributes, or something more specific?  What exactly?  There are tons of
builtins that behave similarly to memcpy/memmove/strcmp/memcmp.

	Jakub
Andrew Pinski - Sept. 3, 2012, 7:34 a.m.
On Sun, Sep 2, 2012 at 11:36 PM, Georg-Johann Lay <gjl@gcc.gnu.org> wrote:
> Andrew Pinski schrieb:
>>
>> Hi,
>>   While fixing some code not to have aliasing violations in it, I can
>> across that some builtins were not causing their arguments or their
>> return values being marked as non-null.  This patch implements just
>> that in VPR while allowing to remove some null pointer checks later
>> on.
>>
>> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>
>> Thanks,
>> Andrew Pinski
>>
>> ChangeLog:
>>         * tree-vrp.c (vrp_stmt_computes_nonzero): Return true for some
>>         builtins (memcpy and memmove).
>>         (infer_value_range): Infer nonzero for some arguments to
>>         some builtins (memcpy, memmove, strcmp and memcmp).
>>
>> testsuite/ChangeLog:
>>         * gcc.dg/tree-ssa/vrp-builtins1.c: New testcase.
>>
>>
>>
>> Index: tree-vrp.c
>> ===================================================================
>> --- tree-vrp.c  (revision 190868)
>> +++ tree-vrp.c  (working copy)
>> @@ -1057,6 +1057,20 @@ vrp_stmt_computes_nonzero (gimple stmt,
>>         }
>>      }
>>  +  /* With some builtins, we can infer if the pointer return value
>> +     will be non null.  */
>> +  if (flag_delete_null_pointer_checks
>> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt)
>> +      && DECL_BUILT_IN_CLASS (gimple_call_fndecl (stmt)) ==
>> BUILT_IN_NORMAL)
>> +    {
>> +      switch (DECL_FUNCTION_CODE (gimple_call_fndecl (stmt)))
>> +       {
>> +         case BUILT_IN_MEMCPY:
>> +         case BUILT_IN_MEMMOVE:
>> +           return true;
>> +       }
>> +    }
>> +
>>    return false;
>>  }
>>  @@ -4231,6 +4245,32 @@ infer_value_range (gimple stmt, tree op,
>>         }
>>      }
>>  +  /* With some builtins, we can infer if the pointer argument
>> +     will be non null.  */
>> +  if (flag_delete_null_pointer_checks
>> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt))
>> +    {
>> +      tree callee = gimple_call_fndecl (stmt);
>> +      if (DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
>> +       {
>> +         switch (DECL_FUNCTION_CODE (callee))
>> +           {
>> +             case BUILT_IN_MEMCPY:
>> +             case BUILT_IN_MEMMOVE:
>> +             case BUILT_IN_STRCMP:
>> +             case BUILT_IN_MEMCMP:
>> +               /* The first and second arguments of memcpy and memmove
>> will be non null after the call. */
>> +               if (gimple_call_arg (stmt, 0) == op
>> +                   || gimple_call_arg (stmt, 1) == op)
>> +                 {
>> +                   *val_p = build_int_cst (TREE_TYPE (op), 0);
>> +                   *comp_code_p = NE_EXPR;
>> +                   return true;
>> +                 }
>> +           }
>> +       }
>> +    }
>> +
>>    return false;
>>  }
>>  Index: testsuite/gcc.dg/tree-ssa/vrp-builtins1.c
>> ===================================================================
>> --- testsuite/gcc.dg/tree-ssa/vrp-builtins1.c   (revision 0)
>> +++ testsuite/gcc.dg/tree-ssa/vrp-builtins1.c   (revision 0)
>> @@ -0,0 +1,30 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-vrp1" } */
>
>
> Shouldn't this xfail for avr and other targets that set
> flag_delete_null_pointer_checks = 0, cf.
> avr.c:avr_option_override() for example.

This is taken care of by the check keeps_null_pointer_checks which
returns true for avr-*-*.

Thanks,
Andrew Pinski


>
> Johann
>
>> +
>> +struct f1
>> +{
>> +char a[4];
>> +};
>> +
>> +int f(int *a, struct f1 b)
>> +{
>> +  int *c = __builtin_memcpy(a, b.a, 4);
>> +  if (c == 0)
>> +    return 0;
>> +  return *a;
>> +}
>> +
>> +
>> +int f1(int *a, struct f1 b)
>> +{
>> +  int *c = __builtin_memcpy(a, b.a, 4);
>> +  if (a == 0)
>> +    return 0;
>> +  return *a;
>> +}
>> +
>> +/* Both the if statements should be folded when the target does not keep
>> around null pointer checks. */
>> +/* { dg-final { scan-tree-dump-times "Folding predicate" 0 "vrp1" {
>> target {   keeps_null_pointer_checks } } } } */
>> +/* { dg-final { scan-tree-dump-times "Folding predicate" 2 "vrp1" {
>> target { ! keeps_null_pointer_checks } } } } */
>> +/* { dg-final { cleanup-tree-dump "vrp1" } } */
>> +
>
>
Andrew Pinski - Sept. 3, 2012, 7:38 a.m.
On Mon, Sep 3, 2012 at 12:03 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Sep 02, 2012 at 10:18:15PM -0700, Andrew Pinski wrote:
>>   While fixing some code not to have aliasing violations in it, I can
>> across that some builtins were not causing their arguments or their
>> return values being marked as non-null.  This patch implements just
>> that in VPR while allowing to remove some null pointer checks later
>> on.
>>
>> OK?  Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>> @@ -1057,6 +1057,20 @@ vrp_stmt_computes_nonzero (gimple stmt,
>>       }
>>      }
>>
>> +  /* With some builtins, we can infer if the pointer return value
>> +     will be non null.  */
>> +  if (flag_delete_null_pointer_checks
>> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt)
>> +      && DECL_BUILT_IN_CLASS (gimple_call_fndecl (stmt)) == BUILT_IN_NORMAL)
>> +    {
>> +      switch (DECL_FUNCTION_CODE (gimple_call_fndecl (stmt)))
>> +     {
>> +       case BUILT_IN_MEMCPY:
>> +       case BUILT_IN_MEMMOVE:
>> +         return true;
>> +     }
>> +    }
>> +
>>    return false;
>>  }
>>
>
> That is too hackish and lists way too few builtins.
> If you rely on nonnull attribute marked builtins, I'd say you want
> flags = gimple_call_return_flags (stmt);
> if ((flags & ERF_RETURNS_ARG)
>     && (flags & ERF_RETURN_ARG_MASK) < gimple_call_num_args (stmt))
>   {
>     /* Test nonnull attribute on the decl, either argument-less or
>        on the (flags & ERF_RETURN_ARG_MASK)th argument.  */
>   }
> Or at least handle builtins e.g. CCP handles as pass-thru arg1:
> BUILT_IN_MEMCPY, BUILT_IN_MEMMOVE, BUILT_IN_MEMSET, BUILT_IN_STRCPY,
> BUILT_IN_STRNCPY, BUILT_IN_MEMCPY_CHK, BUILT_IN_MEMMOVE_CHK,
> BUILT_IN_MEMSET_CHK, BUILT_IN_STRCPY_CHK, BUILT_IN_STRNCPY_CHK
> (which reminds me that some of these apparently aren't marked
> with ATTR_RET1_NOTHROW_NONNULL_LEAF, why?).


I did not know of these attributes and flags on the decl.  Yes I know
it only lists only a few of the builtins.
I will look into those flags and see if I can improve it.

>
>> @@ -4231,6 +4245,32 @@ infer_value_range (gimple stmt, tree op,
>>       }
>>      }
>>
>> +  /* With some builtins, we can infer if the pointer argument
>> +     will be non null.  */
>> +  if (flag_delete_null_pointer_checks
>> +      && is_gimple_call (stmt) && gimple_call_fndecl (stmt))
>> +    {
>> +      tree callee = gimple_call_fndecl (stmt);
>> +      if (DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
>> +     {
>> +       switch (DECL_FUNCTION_CODE (callee))
>> +         {
>> +           case BUILT_IN_MEMCPY:
>> +           case BUILT_IN_MEMMOVE:
>> +           case BUILT_IN_STRCMP:
>> +           case BUILT_IN_MEMCMP:
>> +             /* The first and second arguments of memcpy and memmove will be non null after the call. */
>> +             if (gimple_call_arg (stmt, 0) == op
>> +                 || gimple_call_arg (stmt, 1) == op)
>> +               {
>> +                 *val_p = build_int_cst (TREE_TYPE (op), 0);
>> +                 *comp_code_p = NE_EXPR;
>> +                 return true;
>> +               }
>> +         }
>> +     }
>> +    }
>
> Again, what you are looking for here?  Passing pointers to nonnull
> attributes, or something more specific?  What exactly?  There are tons of
> builtins that behave similarly to memcpy/memmove/strcmp/memcmp.

Passing pointers arguments to nonnull attributed functions.  I will
see if I can use the nonnull attribute on those functions so that this
can be done without checking the builtin functions.

Thanks,
Andrew Pinski

>
>         Jakub

Patch

Index: tree-vrp.c
===================================================================
--- tree-vrp.c	(revision 190868)
+++ tree-vrp.c	(working copy)
@@ -1057,6 +1057,20 @@  vrp_stmt_computes_nonzero (gimple stmt,
 	}
     }
 
+  /* With some builtins, we can infer if the pointer return value
+     will be non null.  */
+  if (flag_delete_null_pointer_checks
+      && is_gimple_call (stmt) && gimple_call_fndecl (stmt)
+      && DECL_BUILT_IN_CLASS (gimple_call_fndecl (stmt)) == BUILT_IN_NORMAL)
+    {
+      switch (DECL_FUNCTION_CODE (gimple_call_fndecl (stmt)))
+	{
+	  case BUILT_IN_MEMCPY:
+	  case BUILT_IN_MEMMOVE:
+	    return true;
+	}
+    }
+
   return false;
 }
 
@@ -4231,6 +4245,32 @@  infer_value_range (gimple stmt, tree op,
 	}
     }
 
+  /* With some builtins, we can infer if the pointer argument
+     will be non null.  */
+  if (flag_delete_null_pointer_checks
+      && is_gimple_call (stmt) && gimple_call_fndecl (stmt))
+    {
+      tree callee = gimple_call_fndecl (stmt);
+      if (DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
+	{
+	  switch (DECL_FUNCTION_CODE (callee))
+	    {
+	      case BUILT_IN_MEMCPY:
+	      case BUILT_IN_MEMMOVE:
+	      case BUILT_IN_STRCMP:
+	      case BUILT_IN_MEMCMP:
+		/* The first and second arguments of memcpy and memmove will be non null after the call. */
+		if (gimple_call_arg (stmt, 0) == op
+		    || gimple_call_arg (stmt, 1) == op)
+		  {
+		    *val_p = build_int_cst (TREE_TYPE (op), 0);
+		    *comp_code_p = NE_EXPR;
+		    return true;
+		  }
+	    }
+	}
+    }
+
   return false;
 }
 
Index: testsuite/gcc.dg/tree-ssa/vrp-builtins1.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/vrp-builtins1.c	(revision 0)
+++ testsuite/gcc.dg/tree-ssa/vrp-builtins1.c	(revision 0)
@@ -0,0 +1,30 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-vrp1" } */
+
+struct f1
+{
+char a[4];
+};
+
+int f(int *a, struct f1 b)
+{
+  int *c = __builtin_memcpy(a, b.a, 4);
+  if (c == 0)
+    return 0;
+  return *a;
+}
+
+
+int f1(int *a, struct f1 b)
+{
+  int *c = __builtin_memcpy(a, b.a, 4);
+  if (a == 0)
+    return 0;
+  return *a;
+}
+
+/* Both the if statements should be folded when the target does not keep around null pointer checks. */
+/* { dg-final { scan-tree-dump-times "Folding predicate" 0 "vrp1" { target {   keeps_null_pointer_checks } } } } */
+/* { dg-final { scan-tree-dump-times "Folding predicate" 2 "vrp1" { target { ! keeps_null_pointer_checks } } } } */
+/* { dg-final { cleanup-tree-dump "vrp1" } } */
+