| Message ID | 20140730165605.GE14543@virgil.suse |
|---|---|
| State | New |
| Headers | show |
Hi, On Wed, Jul 30, 2014 at 06:56:05PM +0200, Martin Jambor wrote: > Hi, > > IPA-CP can wreck havoc to transactional memory support as described in > the summary of the PR in bugzilla. It seems the cause is that IPA-CP > clones of nodes created by trans-mem do not have their tm_clone flag > set. For release branches we have decided to simply disable IPA-CP of > trans-mem clones but for trunk we'd like to avoid this. I am not 100% > sure that just copying the flag is OK but it seems that it works for > the provided testcase and nobody from the trans-mem people has > commented in bugzilla for over a month. So I suggest we commit this > patch and wait and see if something breaks. Hopefully nothing will. > Honza has approved the patch in person and so I have committed it as revision 213666 after re-testing. Hopefully it does not break anything, if it does then read the paragraph above and remember it is not really my fault :-) Martin > Bootstrapped and tested on x86_64-linux. OK for trunk? > > Thanks, > > Martin > > > 2014-07-29 Martin Jambor <mjambor@suse.cz> > > PR ipa/61393 > * cgraphclones.c (cgraph_node::create_clone): Also copy tm_clone. > > diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c > index f097da8..c04b5c8 100644 > --- a/gcc/cgraphclones.c > +++ b/gcc/cgraphclones.c > @@ -423,6 +423,7 @@ cgraph_node::create_clone (tree decl, gcov_type gcov_count, int freq, > new_node->count = count; > new_node->frequency = frequency; > new_node->tp_first_run = tp_first_run; > + new_node->tm_clone = tm_clone; > > new_node->clone.tree_map = NULL; > new_node->clone.args_to_skip = args_to_skip;
Hi Martin, On Wed, Aug 6, 2014 at 4:02 PM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Wed, Jul 30, 2014 at 06:56:05PM +0200, Martin Jambor wrote: >> Hi, >> >> IPA-CP can wreck havoc to transactional memory support as described in >> the summary of the PR in bugzilla. It seems the cause is that IPA-CP >> clones of nodes created by trans-mem do not have their tm_clone flag >> set. For release branches we have decided to simply disable IPA-CP of >> trans-mem clones but for trunk we'd like to avoid this. I am not 100% >> sure that just copying the flag is OK but it seems that it works for >> the provided testcase and nobody from the trans-mem people has >> commented in bugzilla for over a month. So I suggest we commit this >> patch and wait and see if something breaks. Hopefully nothing will. >> > > Honza has approved the patch in person and so I have committed it as > revision 213666 after re-testing. Hopefully it does not break > anything, if it does then read the paragraph above and remember it is > not really my fault :-) Thanks for fixing this! Copying the flag seems to me the good solution. tm_clone flag indicates that this is an instrumented function. It is used for different part in trans-mem but particularly to find tm region to instrument. However, I am wondering if we should not call record_tm_clone_pair if it duplicates a tm_clone. libitm needs to find the clone of particular function. So the question is: Can the compiler replace a function pointer by its corresponding constprop function pointer in some cases? -- Patrick Marlier
Hi, On Thu, Aug 07, 2014 at 08:10:51AM +0200, Patrick Marlier wrote: > Hi Martin, > > On Wed, Aug 6, 2014 at 4:02 PM, Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > > > On Wed, Jul 30, 2014 at 06:56:05PM +0200, Martin Jambor wrote: > >> Hi, > >> > >> IPA-CP can wreck havoc to transactional memory support as described in > >> the summary of the PR in bugzilla. It seems the cause is that IPA-CP > >> clones of nodes created by trans-mem do not have their tm_clone flag > >> set. For release branches we have decided to simply disable IPA-CP of > >> trans-mem clones but for trunk we'd like to avoid this. I am not 100% > >> sure that just copying the flag is OK but it seems that it works for > >> the provided testcase and nobody from the trans-mem people has > >> commented in bugzilla for over a month. So I suggest we commit this > >> patch and wait and see if something breaks. Hopefully nothing will. > >> > > > > Honza has approved the patch in person and so I have committed it as > > revision 213666 after re-testing. Hopefully it does not break > > anything, if it does then read the paragraph above and remember it is > > not really my fault :-) > > Thanks for fixing this! > Copying the flag seems to me the good solution. tm_clone flag > indicates that this is an instrumented function. It is used for > different part in trans-mem but particularly to find tm region to > instrument. > However, I am wondering if we should not call record_tm_clone_pair if > it duplicates a tm_clone. libitm needs to find the clone of particular > function. So the question is: Can the compiler replace a function > pointer by its corresponding constprop function pointer in some cases? As far as I understand your question, the answer is no. When IPA-CP creates a clone, it then redirects any number of call graph edges to that clone, which will eventually make the corresponding direct calls call the new clone (modulo inlining) and that is how the clone is reachable. There should be no addresses of any IPA-CP clone flying around anywhere. If address of the original function is taken, it is not being tracked for the purposes of figuring out whether the new clone could be used instead. Thanks, Martin
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c index f097da8..c04b5c8 100644 --- a/gcc/cgraphclones.c +++ b/gcc/cgraphclones.c @@ -423,6 +423,7 @@ cgraph_node::create_clone (tree decl, gcov_type gcov_count, int freq, new_node->count = count; new_node->frequency = frequency; new_node->tp_first_run = tp_first_run; + new_node->tm_clone = tm_clone; new_node->clone.tree_map = NULL; new_node->clone.args_to_skip = args_to_skip;