| Message ID | 201101041303.31897.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
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 >
> 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
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 >
> 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 > >
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 >> > >
> 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 > >> > > >
> 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.
> > 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
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; }