Patchwork Fix PR tree-optimization/47056

login
register
mail settings
Submitter Eric Botcazou
Date Jan. 4, 2011, 12:03 p.m.
Message ID <201101041303.31897.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/77457/
State New
Headers show

Comments

Eric Botcazou - Jan. 4, 2011, 12:03 p.m.
Hi,

this is the link failure of 10 ACATS tests on IA64/Linux.  The problem is that 
reachability analysis fails to see that some functions are needed because 
they are referenced in the code.  More precisely, CCP turns:

  pr47056___alignment.46_308 = pr47056___alignment;
  D.2264_309 = MEM[(struct  *)pr47056___alignment.46_308].D.227;

into

  D.2264_309 = MEM[(struct  *)pr47056___alignment].D.227;

(pr47056___alignment being a function) and while walk_stmt_load_store_addr_ops
recognizes the former as an address-of pattern (the & is implicit for
functions), it doesn't for the latter (unlike for TARGET_MEM_REF) so
build_cgraph_edges is fooled and missed the reference to pr47056___alignment.

The attached patch adds a specific kludge to mark_load for this case.  It also 
removes bogus ATTRIBUTE_UNUSED markers and makes mark_store pass the STMT to 
ipa_record_reference like the 2 other functions (mark_load and mark_address).

It passed regression testing on x86.  OK after a complete testing cycle?


2011-01-04  Eric Botcazou  <ebotcazou@adacore.com>

	PR tree-optimization/47056
	* cgraphbuild.c (mark_address): Remove ATTRIBUTE_UNUSED markers.
	(mark_load): Likewise.  Handle FUNCTION_DECL specially.
	(mark_store): Likewise.  Pass STMT to ipa_record_reference.
Richard Guenther - Jan. 4, 2011, 4:20 p.m.
On Tue, Jan 4, 2011 at 1:03 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is the link failure of 10 ACATS tests on IA64/Linux.  The problem is that
> reachability analysis fails to see that some functions are needed because
> they are referenced in the code.  More precisely, CCP turns:
>
>  pr47056___alignment.46_308 = pr47056___alignment;
>  D.2264_309 = MEM[(struct  *)pr47056___alignment.46_308].D.227;
>
> into
>
>  D.2264_309 = MEM[(struct  *)pr47056___alignment].D.227;
>
> (pr47056___alignment being a function) and while walk_stmt_load_store_addr_ops
> recognizes the former as an address-of pattern (the & is implicit for
> functions), it doesn't for the latter (unlike for TARGET_MEM_REF) so

Yeah, the TARGET_MEM_REF thing looks from before the MEM_REF
merge.  It shouldn't recognize it as an address either.

> build_cgraph_edges is fooled and missed the reference to pr47056___alignment.
>
> The attached patch adds a specific kludge to mark_load for this case.  It also
> removes bogus ATTRIBUTE_UNUSED markers and makes mark_store pass the STMT to
> ipa_record_reference like the 2 other functions (mark_load and mark_address).
>
> It passed regression testing on x86.  OK after a complete testing cycle?

I don't know if manipulating the descriptor would count effectively as
an address-taking of the function.  Maybe Honza can comment.  Otherwise
the patch is ok.

Thanks,
Richard.

>
> 2011-01-04  Eric Botcazou  <ebotcazou@adacore.com>
>
>        PR tree-optimization/47056
>        * cgraphbuild.c (mark_address): Remove ATTRIBUTE_UNUSED markers.
>        (mark_load): Likewise.  Handle FUNCTION_DECL specially.
>        (mark_store): Likewise.  Pass STMT to ipa_record_reference.
>
>
> --
> Eric Botcazou
>
Jan Hubicka - Jan. 4, 2011, 4:26 p.m.
> On Tue, Jan 4, 2011 at 1:03 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> > Hi,
> >
> > this is the link failure of 10 ACATS tests on IA64/Linux.  The problem is that
> > reachability analysis fails to see that some functions are needed because
> > they are referenced in the code.  More precisely, CCP turns:
> >
> >  pr47056___alignment.46_308 = pr47056___alignment;
> >  D.2264_309 = MEM[(struct  *)pr47056___alignment.46_308].D.227;
> >
> > into
> >
> >  D.2264_309 = MEM[(struct  *)pr47056___alignment].D.227;
> >
> > (pr47056___alignment being a function) and while walk_stmt_load_store_addr_ops
> > recognizes the former as an address-of pattern (the & is implicit for
> > functions), it doesn't for the latter (unlike for TARGET_MEM_REF) so
> 
> Yeah, the TARGET_MEM_REF thing looks from before the MEM_REF
> merge.  It shouldn't recognize it as an address either.
> 
> > build_cgraph_edges is fooled and missed the reference to pr47056___alignment.
> >
> > The attached patch adds a specific kludge to mark_load for this case.  It also
> > removes bogus ATTRIBUTE_UNUSED markers and makes mark_store pass the STMT to
> > ipa_record_reference like the 2 other functions (mark_load and mark_address).
> >
> > It passed regression testing on x86.  OK after a complete testing cycle?
> 
> I don't know if manipulating the descriptor would count effectively as
> an address-taking of the function.  Maybe Honza can comment.  Otherwise
> the patch is ok.

Hi,
I am having problems to parse the gimple. pr47056___alignment is function
and MEM[(struct | *)pr47056___alignment].D.227 is read from the  function's text
(i.e. something quite underfined?).
If it is so, then indeed this should count as address taken in cgraph sense.

Honza
Richard Guenther - Jan. 4, 2011, 4:30 p.m.
2011/1/4 Jan Hubicka <hubicka@ucw.cz>:
>> On Tue, Jan 4, 2011 at 1:03 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> > Hi,
>> >
>> > this is the link failure of 10 ACATS tests on IA64/Linux.  The problem is that
>> > reachability analysis fails to see that some functions are needed because
>> > they are referenced in the code.  More precisely, CCP turns:
>> >
>> >  pr47056___alignment.46_308 = pr47056___alignment;
>> >  D.2264_309 = MEM[(struct  *)pr47056___alignment.46_308].D.227;
>> >
>> > into
>> >
>> >  D.2264_309 = MEM[(struct  *)pr47056___alignment].D.227;
>> >
>> > (pr47056___alignment being a function) and while walk_stmt_load_store_addr_ops
>> > recognizes the former as an address-of pattern (the & is implicit for
>> > functions), it doesn't for the latter (unlike for TARGET_MEM_REF) so
>>
>> Yeah, the TARGET_MEM_REF thing looks from before the MEM_REF
>> merge.  It shouldn't recognize it as an address either.
>>
>> > build_cgraph_edges is fooled and missed the reference to pr47056___alignment.
>> >
>> > The attached patch adds a specific kludge to mark_load for this case.  It also
>> > removes bogus ATTRIBUTE_UNUSED markers and makes mark_store pass the STMT to
>> > ipa_record_reference like the 2 other functions (mark_load and mark_address).
>> >
>> > It passed regression testing on x86.  OK after a complete testing cycle?
>>
>> I don't know if manipulating the descriptor would count effectively as
>> an address-taking of the function.  Maybe Honza can comment.  Otherwise
>> the patch is ok.
>
> Hi,
> I am having problems to parse the gimple. pr47056___alignment is function
> and MEM[(struct | *)pr47056___alignment].D.227 is read from the  function's text
> (i.e. something quite underfined?).
> If it is so, then indeed this should count as address taken in cgraph sense.

We print &function-decl as just function-decl to confuse us ;)  (because
of calls I guess)

Richard.

>
> Honza
>
Jan Hubicka - Jan. 4, 2011, 5:07 p.m.
> 2011/1/4 Jan Hubicka <hubicka@ucw.cz>:
> >> On Tue, Jan 4, 2011 at 1:03 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> >> > Hi,
> >> >
> >> > this is the link failure of 10 ACATS tests on IA64/Linux.  The problem is that
> >> > reachability analysis fails to see that some functions are needed because
> >> > they are referenced in the code.  More precisely, CCP turns:
> >> >
> >> >  pr47056___alignment.46_308 = pr47056___alignment;
> >> >  D.2264_309 = MEM[(struct  *)pr47056___alignment.46_308].D.227;
> >> >
> >> > into
> >> >
> >> >  D.2264_309 = MEM[(struct  *)pr47056___alignment].D.227;
> >> >
> >> > (pr47056___alignment being a function) and while walk_stmt_load_store_addr_ops
> >> > recognizes the former as an address-of pattern (the & is implicit for
> >> > functions), it doesn't for the latter (unlike for TARGET_MEM_REF) so
> >>
> >> Yeah, the TARGET_MEM_REF thing looks from before the MEM_REF
> >> merge.  It shouldn't recognize it as an address either.
> >>
> >> > build_cgraph_edges is fooled and missed the reference to pr47056___alignment.
> >> >
> >> > The attached patch adds a specific kludge to mark_load for this case.  It also
> >> > removes bogus ATTRIBUTE_UNUSED markers and makes mark_store pass the STMT to
> >> > ipa_record_reference like the 2 other functions (mark_load and mark_address).
> >> >
> >> > It passed regression testing on x86.  OK after a complete testing cycle?
> >>
> >> I don't know if manipulating the descriptor would count effectively as
> >> an address-taking of the function.  Maybe Honza can comment.  Otherwise
> >> the patch is ok.
> >
> > Hi,
> > I am having problems to parse the gimple. pr47056___alignment is function
> > and MEM[(struct | *)pr47056___alignment].D.227 is read from the  function's text
> > (i.e. something quite underfined?).
> > If it is so, then indeed this should count as address taken in cgraph sense.
> 
> We print &function-decl as just function-decl to confuse us ;)  (because
> of calls I guess)

OK.
There is bit of quirk that cgraph is not really considering posibility of direct
read or write to/from function, only to/from var.  I guess modeling all of these
as dereferences is safe for reachability analysis and we don't need to care
about efectivity since the code is undefined anyway.
(we lose some code quality by considering such function as having address taken,
but I don't think we care.  Adding read/write edges for functions is possible
but would just confuse prople IMO)

Honza
> 
> Richard.
> 
> >
> > Honza
> >
Richard Guenther - Jan. 5, 2011, 10:53 a.m.
2011/1/4 Jan Hubicka <hubicka@ucw.cz>:
>> 2011/1/4 Jan Hubicka <hubicka@ucw.cz>:
>> >> On Tue, Jan 4, 2011 at 1:03 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> >> > Hi,
>> >> >
>> >> > this is the link failure of 10 ACATS tests on IA64/Linux.  The problem is that
>> >> > reachability analysis fails to see that some functions are needed because
>> >> > they are referenced in the code.  More precisely, CCP turns:
>> >> >
>> >> >  pr47056___alignment.46_308 = pr47056___alignment;
>> >> >  D.2264_309 = MEM[(struct  *)pr47056___alignment.46_308].D.227;
>> >> >
>> >> > into
>> >> >
>> >> >  D.2264_309 = MEM[(struct  *)pr47056___alignment].D.227;
>> >> >
>> >> > (pr47056___alignment being a function) and while walk_stmt_load_store_addr_ops
>> >> > recognizes the former as an address-of pattern (the & is implicit for
>> >> > functions), it doesn't for the latter (unlike for TARGET_MEM_REF) so
>> >>
>> >> Yeah, the TARGET_MEM_REF thing looks from before the MEM_REF
>> >> merge.  It shouldn't recognize it as an address either.
>> >>
>> >> > build_cgraph_edges is fooled and missed the reference to pr47056___alignment.
>> >> >
>> >> > The attached patch adds a specific kludge to mark_load for this case.  It also
>> >> > removes bogus ATTRIBUTE_UNUSED markers and makes mark_store pass the STMT to
>> >> > ipa_record_reference like the 2 other functions (mark_load and mark_address).
>> >> >
>> >> > It passed regression testing on x86.  OK after a complete testing cycle?
>> >>
>> >> I don't know if manipulating the descriptor would count effectively as
>> >> an address-taking of the function.  Maybe Honza can comment.  Otherwise
>> >> the patch is ok.
>> >
>> > Hi,
>> > I am having problems to parse the gimple. pr47056___alignment is function
>> > and MEM[(struct | *)pr47056___alignment].D.227 is read from the  function's text
>> > (i.e. something quite underfined?).
>> > If it is so, then indeed this should count as address taken in cgraph sense.
>>
>> We print &function-decl as just function-decl to confuse us ;)  (because
>> of calls I guess)
>
> OK.
> There is bit of quirk that cgraph is not really considering posibility of direct
> read or write to/from function, only to/from var.  I guess modeling all of these
> as dereferences is safe for reachability analysis and we don't need to care
> about efectivity since the code is undefined anyway.
> (we lose some code quality by considering such function as having address taken,
> but I don't think we care.  Adding read/write edges for functions is possible
> but would just confuse prople IMO)

OTOH, is writing/reading to/from a function descriptor (or text) not rather
similar to a call to it than to taking its address?  I think making it
address-taken
will make all indirect calls possibly calling this function whereas "calling it"
would merely make sure it will stay around.

Richard.

> Honza
>>
>> Richard.
>>
>> >
>> > Honza
>> >
>
Jan Hubicka - Jan. 5, 2011, 11:03 a.m.
> OTOH, is writing/reading to/from a function descriptor (or text) not rather
> similar to a call to it than to taking its address?  I think making it

Adding fake calls at places where call does not happen is also bad idea.  IPA
passes expect call edges to correspond to call statemnets and will try to do
stuff with them.

I didn't noticed it is with ia64 and function descriptors.  Here read from
function descriptor indeed makes more sense then read from random place in text
segment on other arches.  What exactly is the code sequence doing with the
descritor?  We might add read edges for functions for this reason, but it seems
to me that only reason to read function descriptor is to call it indirectly
later, so it is equivalent of taking address anyway?


Honza
> address-taken
> will make all indirect calls possibly calling this function whereas "calling it"
> would merely make sure it will stay around.
> 
> Richard.
> 
> > Honza
> >>
> >> Richard.
> >>
> >> >
> >> > Honza
> >> >
> >
Eric Botcazou - Jan. 5, 2011, 11:11 a.m.
> I didn't noticed it is with ia64 and function descriptors.  Here read from
> function descriptor indeed makes more sense then read from random place in
> text segment on other arches.  What exactly is the code sequence doing with
> the descritor?

Copying it from a static to a dynamic vtable.

> We might add read edges for functions for this reason, but 
> it seems to me that only reason to read function descriptor is to call it
> indirectly later, so it is equivalent of taking address anyway?

Yes, exactly.
Jan Hubicka - Jan. 5, 2011, 11:15 a.m.
> > I didn't noticed it is with ia64 and function descriptors.  Here read from
> > function descriptor indeed makes more sense then read from random place in
> > text segment on other arches.  What exactly is the code sequence doing with
> > the descritor?
> 
> Copying it from a static to a dynamic vtable.
> 
> > We might add read edges for functions for this reason, but 
> > it seems to me that only reason to read function descriptor is to call it
> > indirectly later, so it is equivalent of taking address anyway?
> 
> Yes, exactly.

Agreed, adding address taken edge for dynamic vtable is the right thing...

Honza
> 
> -- 
> Eric Botcazou

Patch

Index: cgraphbuild.c
===================================================================
--- cgraphbuild.c	(revision 168391)
+++ cgraphbuild.c	(working copy)
@@ -234,8 +234,7 @@  compute_call_stmt_bb_frequency (tree dec
 /* Mark address taken in STMT.  */
 
 static bool
-mark_address (gimple stmt ATTRIBUTE_UNUSED, tree addr,
-	      void *data ATTRIBUTE_UNUSED)
+mark_address (gimple stmt, tree addr, void *data)
 {
   addr = get_base_address (addr);
   if (TREE_CODE (addr) == FUNCTION_DECL)
@@ -268,12 +267,21 @@  mark_address (gimple stmt ATTRIBUTE_UNUS
 /* Mark load of T.  */
 
 static bool
-mark_load (gimple stmt ATTRIBUTE_UNUSED, tree t,
-	   void *data ATTRIBUTE_UNUSED)
+mark_load (gimple stmt, tree t, void *data)
 {
   t = get_base_address (t);
-  if (t && TREE_CODE (t) == VAR_DECL
-      && (TREE_STATIC (t) || DECL_EXTERNAL (t)))
+  if (t && TREE_CODE (t) == FUNCTION_DECL)
+    {
+      /* ??? This can happen on platforms with descriptors when these are
+	 directly manipulated in the code.  Pretend that it's an address.  */
+      struct cgraph_node *node = cgraph_node (t);
+      cgraph_mark_address_taken_node (node);
+      ipa_record_reference ((struct cgraph_node *)data, NULL,
+			    node, NULL,
+			    IPA_REF_ADDR, stmt);
+    }
+  else if (t && TREE_CODE (t) == VAR_DECL
+	   && (TREE_STATIC (t) || DECL_EXTERNAL (t)))
     {
       struct varpool_node *vnode = varpool_node (t);
       int walk_subtrees;
@@ -293,8 +301,7 @@  mark_load (gimple stmt ATTRIBUTE_UNUSED,
 /* Mark store of T.  */
 
 static bool
-mark_store (gimple stmt ATTRIBUTE_UNUSED, tree t,
-	    void *data ATTRIBUTE_UNUSED)
+mark_store (gimple stmt, tree t, void *data)
 {
   t = get_base_address (t);
   if (t && TREE_CODE (t) == VAR_DECL
@@ -310,7 +317,7 @@  mark_store (gimple stmt ATTRIBUTE_UNUSED
 	vnode = vnode->extra_name;
       ipa_record_reference ((struct cgraph_node *)data, NULL,
 			    NULL, vnode,
-			    IPA_REF_STORE, NULL);
+			    IPA_REF_STORE, stmt);
      }
   return false;
 }