[RFC,IPA-VRP] ADDR_EXPR and nonnull

Message ID 0ef084d3-b9fa-4623-d353-c14a16c0b907@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Oct. 21, 2016, 7:38 a.m.
On 21/10/16 18:16, Richard Biener wrote:
> On Fri, 21 Oct 2016, kugan wrote:

>

>> Hi,

>>

>> On 20/10/16 23:15, Jan Hubicka wrote:

>>>> Hi Richard,

>>>>

>>>>

>>>> On 20/10/16 18:41, Richard Biener wrote:

>>>>> On Thu, 20 Oct 2016, kugan wrote:

>>>>>

>>>>>>

>>>>>>

>>>>>> On 20/10/16 01:26, Jan Hubicka wrote:

>>>>>>>>> Would excluding weak symbols (I believe I can check DECL_WEAK

>>>>>>>>> for this)

>>>>>>>>> good

>>>>>>>>> enough. Or looking for acceptable subset would work?

>>>>>>>>

>>>>>>>> I think we should add a symtab helper to tell if address_nonzero_p

>>>>>>>> (if

>>>>>>>> that doesn't aleady exist).

>>>>>>>

>>>>>>> We have node->nonzero_address()

>>>>>>

>>>>>> Thanks for the pointer. Here is an attempt. Does this look OK?

>>>>>>

>>>>>> +	  if (TREE_CODE (arg) == ADDR_EXPR)

>>>>>> +	    {

>>>>>> +	      /* See if the AADR_EXPR is nonnull.  */

>>>>>> +	      varpool_node *node = NULL;

>>>>>> +	      tree base = TREE_OPERAND (arg, 0);

>>>>>> +	      base = get_base_address (base);

>>>>>> +

>>>>>> +	      if (decl_address_ip_invariant_p (base)

>>>>>> +		  || !is_global_var (base))

>>>>>> +		{

>>>>>> +		  /* If the symbol address is local or

>>>>>> +		     constant.  */

>>>>>

>>>>> "constant" doesn't matter.  You want

>>>>>

>>>>>   if (TREE_CODE (base) == CONST_DECL

>>>>> || TREE_CODE (base) == PARM_DECL

>>>>> || TREE_CODE (base) == RESULT_DECL)

>>>>>  addr_nonzero = true;

>>>>>   else if (VAR_P (base))

>>> Better to check decl_in_symtab_p (decl)

>>>>>     addr_nonzero = ! is_global_var (base) || (varpool_node::get

>>>>> (base)->nonzero_address ());

>>> and symtab_node::get.

>>>

>>> I wonder if we can't unify the logic with tree_expr_nonzero_warnv_p

>>> and corresponding vrp code?

>> Are you saying that we should export tree_expr_nonzero_warnv_p and use here

>> with the logic from above added to tree_expr_nonzero_warnv_p as shown in the

>> attached patch?

>

> tree_single_nonzero_warnv_p already handles this correctly via

> maybe_nonzero_address.  And tree_single_nonzero_warnv_p is already

> exported.

Indeed. maybe_nonzero_address does look at node->nonzero_address () but 
we still seems to miss TREE_CODE (base) == PARM_DECL and TREE_CODE 
(base) == RESULT_DECL.

Does the attached patch looks OK if no regressions?

Thanks,
Kugan


>

> Richard.

>

>> Thanks,

>> Kugan

>>

>>>

>>> Otherwise the patch looks fine to me.

>>> Honza

>>>

>>

>

Comments

Richard Biener Oct. 21, 2016, 8:08 a.m. | #1
On Fri, 21 Oct 2016, kugan wrote:

> 

> 

> On 21/10/16 18:16, Richard Biener wrote:

> > On Fri, 21 Oct 2016, kugan wrote:

> > 

> > > Hi,

> > > 

> > > On 20/10/16 23:15, Jan Hubicka wrote:

> > > > > Hi Richard,

> > > > > 

> > > > > 

> > > > > On 20/10/16 18:41, Richard Biener wrote:

> > > > > > On Thu, 20 Oct 2016, kugan wrote:

> > > > > > 

> > > > > > > 

> > > > > > > 

> > > > > > > On 20/10/16 01:26, Jan Hubicka wrote:

> > > > > > > > > > Would excluding weak symbols (I believe I can check

> > > > > > > > > > DECL_WEAK

> > > > > > > > > > for this)

> > > > > > > > > > good

> > > > > > > > > > enough. Or looking for acceptable subset would work?

> > > > > > > > > 

> > > > > > > > > I think we should add a symtab helper to tell if

> > > > > > > > > address_nonzero_p

> > > > > > > > > (if

> > > > > > > > > that doesn't aleady exist).

> > > > > > > > 

> > > > > > > > We have node->nonzero_address()

> > > > > > > 

> > > > > > > Thanks for the pointer. Here is an attempt. Does this look OK?

> > > > > > > 

> > > > > > > +	  if (TREE_CODE (arg) == ADDR_EXPR)

> > > > > > > +	    {

> > > > > > > +	      /* See if the AADR_EXPR is nonnull.  */

> > > > > > > +	      varpool_node *node = NULL;

> > > > > > > +	      tree base = TREE_OPERAND (arg, 0);

> > > > > > > +	      base = get_base_address (base);

> > > > > > > +

> > > > > > > +	      if (decl_address_ip_invariant_p (base)

> > > > > > > +		  || !is_global_var (base))

> > > > > > > +		{

> > > > > > > +		  /* If the symbol address is local or

> > > > > > > +		     constant.  */

> > > > > > 

> > > > > > "constant" doesn't matter.  You want

> > > > > > 

> > > > > >   if (TREE_CODE (base) == CONST_DECL

> > > > > > || TREE_CODE (base) == PARM_DECL

> > > > > > || TREE_CODE (base) == RESULT_DECL)

> > > > > >  addr_nonzero = true;

> > > > > >   else if (VAR_P (base))

> > > > Better to check decl_in_symtab_p (decl)

> > > > > >     addr_nonzero = ! is_global_var (base) || (varpool_node::get

> > > > > > (base)->nonzero_address ());

> > > > and symtab_node::get.

> > > > 

> > > > I wonder if we can't unify the logic with tree_expr_nonzero_warnv_p

> > > > and corresponding vrp code?

> > > Are you saying that we should export tree_expr_nonzero_warnv_p and use

> > > here

> > > with the logic from above added to tree_expr_nonzero_warnv_p as shown in

> > > the

> > > attached patch?

> > 

> > tree_single_nonzero_warnv_p already handles this correctly via

> > maybe_nonzero_address.  And tree_single_nonzero_warnv_p is already

> > exported.

> Indeed. maybe_nonzero_address does look at node->nonzero_address () but we

> still seems to miss TREE_CODE (base) == PARM_DECL and TREE_CODE (base) ==

> RESULT_DECL.


Should be handled by

        /* Function local objects are never NULL.  */
        if (DECL_P (base)
            && (DECL_CONTEXT (base)
                && TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL
                && auto_var_in_fn_p (base, DECL_CONTEXT (base))))
          return true;

> Does the attached patch looks OK if no regressions?


+         else if (tree_single_nonzero_warnv_p (arg, &strict_overflow))
+           {
+             if (!strict_overflow)
+               addr_nonzero = true;

you don't need to care for strict_overflow here.

The patch is ok with that removed and the fold-const.c changes dropped.

Richard.

> Thanks,

> Kugan

> 

> 

> > 

> > Richard.

> > 

> > > Thanks,

> > > Kugan

> > > 

> > > > 

> > > > Otherwise the patch looks fine to me.

> > > > Honza

> > > > 

> > > 

> > 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Patch hide | download patch | download mbox

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 89ed89d..3c1803f 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -13402,7 +13402,9 @@  tree_single_nonzero_warnv_p (tree t, bool *strict_overflow_p)
 	  return true;
 
 	/* Constants are never weak.  */
-	if (CONSTANT_CLASS_P (base))
+	if (CONSTANT_CLASS_P (base)
+	    || TREE_CODE (base) == PARM_DECL
+	    || TREE_CODE (base) == RESULT_DECL)
 	  return true;
 
 	return false;
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 353b638..939fd1d 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1670,9 +1670,20 @@  ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 
       if (POINTER_TYPE_P (TREE_TYPE (arg)))
 	{
+	  bool addr_nonzero = false;
+	  bool strict_overflow = false;
+
 	  if (TREE_CODE (arg) == SSA_NAME
 	      && param_type
 	      && get_ptr_nonnull (arg))
+	    addr_nonzero = true;
+	  else if (tree_single_nonzero_warnv_p (arg, &strict_overflow))
+	    {
+	      if (!strict_overflow)
+		addr_nonzero = true;
+	    }
+
+	  if (addr_nonzero)
 	    {
 	      jfunc->vr_known = true;
 	      jfunc->m_vr.type = VR_ANTI_RANGE;
diff --git a/gcc/testsuite/gcc.dg/ipa/vrp5.c b/gcc/testsuite/gcc.dg/ipa/vrp5.c
index e69de29..571798d 100644
--- a/gcc/testsuite/gcc.dg/ipa/vrp5.c
+++ b/gcc/testsuite/gcc.dg/ipa/vrp5.c
@@ -0,0 +1,34 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-cp-details -fdump-tree-vrp1" } */
+
+static __attribute__((noinline, noclone))
+int foo (int *p)
+{
+  if (!p)
+    return 0;
+  *p = 1;
+}
+
+struct st
+{
+  int a;
+  int b;
+};
+
+int arr1[10];
+int a;
+int bar (struct st *s)
+{
+  int arr2[10];
+  int b;
+  if (!s)
+    return 0;
+  foo (&s->a);
+  foo (&a);
+  foo (&b);
+  foo (&arr1[1]);
+  foo (&arr2[1]);
+}
+
+/* { dg-final { scan-ipa-dump "Setting nonnull for 0" "cp" } } */
+/* { dg-final { scan-tree-dump-times "if" 1 "vrp1" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/vrp6.c b/gcc/testsuite/gcc.dg/ipa/vrp6.c
index e69de29..971db44 100644
--- a/gcc/testsuite/gcc.dg/ipa/vrp6.c
+++ b/gcc/testsuite/gcc.dg/ipa/vrp6.c
@@ -0,0 +1,34 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-cp-details -fdump-tree-vrp1" } */
+
+static __attribute__((noinline, noclone))
+int foo (int *p)
+{
+  if (!p)
+    return 0;
+  *p = 1;
+}
+
+struct st
+{
+  int a;
+  int b;
+};
+
+struct st s2;
+int a;
+int bar (struct st *s)
+{
+  struct st s3;
+  int b;
+  if (!s)
+    return 0;
+  foo (&s->a);
+  foo (&s2.a);
+  foo (&s3.a);
+  foo (&a);
+  foo (&b);
+}
+
+/* { dg-final { scan-ipa-dump "Setting nonnull for 0" "cp" } } */
+/* { dg-final { scan-tree-dump-times "if" 1 "vrp1" } } */