Patchwork shrink-wrapping duplicates BBs across partitions.

login
register
mail settings
Submitter Christian Bruel
Date Sept. 12, 2012, 11:47 a.m.
Message ID <50507644.7090503@st.com>
Download mbox | patch
Permalink /patch/183340/
State New
Headers show

Comments

Christian Bruel - Sept. 12, 2012, 11:47 a.m.
The problem stems from tree-ssa-tail-merge that breaks bb->count, The
CFG looks like

     2
   /  \
  /    6
 5 (0) |
 |     3 <-----
 |    /   \   |
 |   7 (1)  8 -
 | /
  4 (1)

(in parenthesis the bb->count from gcov)

     2
   /  \
  /    6
 /      |
 |      3 <--
 |     / |  |
 5 (0)   8 --
 |
 |
  4 (1)

so 5 and 4 are now in different partitions, producing an assertion
because there is no EDGE_CROSSING between them.

I can see 3 solutions to this

1) merge the BB counts in tree-ssa-tail-merge.c, so 5 is in the same
partition that 4

2) don't tail-merge blocks that belong to different partitions.

3) add a EDGE_CROSSING flag on the edge between 4 and 5.

1) fixes the problem, so 5 and 4 are now in the same partition. The fix
is quite trivial, as with attached.

the other solution 2) is more conservative, and also fixes the problem.

I don't think 3) is necessary.

more ideas ?

thanks,

Christian


On 09/11/2012 06:21 PM, Jakub Jelinek wrote:
> On Tue, Sep 11, 2012 at 05:40:30PM +0200, Steven Bosscher wrote:
>> On Tue, Sep 11, 2012 at 5:31 PM, Christian Bruel <christian.bruel@st.com> wrote:
>>> Actually, the edge is fairly simple. I have
>>>
>>> BB5 (BB_COLD_PARTITION) -> BB10 (BB_HOT_PARTITION) -> EXIT
>>>
>>> and BB10 has no other incoming edges. and we are duplicating it.
>>
>> That is wrong, should never happen. Is there a test case to play with?
>> It'd be good to have a PR for this.
> 
> Isn't that the standard case when !HAVE_return ?  Then you can have only a
> single return through epilogue, and when the epilogue is in the hot
> partition, even if cold code is returning, it needs to jump to the epilogue.
> 
> 	Jakub
>
Andrew Pinski - Sept. 12, 2012, 9:19 p.m.
On Wed, Sep 12, 2012 at 4:47 AM, Christian Bruel <christian.bruel@st.com> wrote:
> The problem stems from tree-ssa-tail-merge that breaks bb->count, The
> CFG looks like
>
>      2
>    /  \
>   /    6
>  5 (0) |
>  |     3 <-----
>  |    /   \   |
>  |   7 (1)  8 -
>  | /
>   4 (1)
>
> (in parenthesis the bb->count from gcov)
>
>      2
>    /  \
>   /    6
>  /      |
>  |      3 <--
>  |     / |  |
>  5 (0)   8 --
>  |
>  |
>   4 (1)
>
> so 5 and 4 are now in different partitions, producing an assertion
> because there is no EDGE_CROSSING between them.
>
> I can see 3 solutions to this
>
> 1) merge the BB counts in tree-ssa-tail-merge.c, so 5 is in the same
> partition that 4

This looks correct as we already do that for the frequency.

>
> 2) don't tail-merge blocks that belong to different partitions.

I don't think you detect this on the tree level as the partitioning
has not happened yet.

Thanks,
Andrew Pinski

>
> 3) add a EDGE_CROSSING flag on the edge between 4 and 5.
>
> 1) fixes the problem, so 5 and 4 are now in the same partition. The fix
> is quite trivial, as with attached.
>
> the other solution 2) is more conservative, and also fixes the problem.
>
> I don't think 3) is necessary.
>
> more ideas ?
>
> thanks,
>
> Christian
>
>
> On 09/11/2012 06:21 PM, Jakub Jelinek wrote:
>> On Tue, Sep 11, 2012 at 05:40:30PM +0200, Steven Bosscher wrote:
>>> On Tue, Sep 11, 2012 at 5:31 PM, Christian Bruel <christian.bruel@st.com> wrote:
>>>> Actually, the edge is fairly simple. I have
>>>>
>>>> BB5 (BB_COLD_PARTITION) -> BB10 (BB_HOT_PARTITION) -> EXIT
>>>>
>>>> and BB10 has no other incoming edges. and we are duplicating it.
>>>
>>> That is wrong, should never happen. Is there a test case to play with?
>>> It'd be good to have a PR for this.
>>
>> Isn't that the standard case when !HAVE_return ?  Then you can have only a
>> single return through epilogue, and when the epilogue is in the hot
>> partition, even if cold code is returning, it needs to jump to the epilogue.
>>
>>       Jakub
>>
Christian Bruel - Sept. 13, 2012, 7:01 a.m.
>> 1) fixes the problem, so 5 and 4 are now in the same partition. The fix
>> is quite trivial, as with attached.
> 
> That looks obviously correct to me. I can't approve it, but I'd have
> committed it as obvious.
> 

Thanks, I'll  make the formal request, after checking forthe unexpected
side effects

> 
> 
>> I don't think 3) is necessary.
> 
> I think it *is* necessary. From cfg-flags.def:
> /* Edge crosses between hot and cold sections, when we do partitioning.
>    This flag is only used for the RTL CFG.  */
> DEF_EDGE_FLAG(CROSSING, 11)
> 
> Edge 5-4 is a crossing edge, so EDGE_CROSSING should be set.

OK, I'll try to investigate this while it's hot and before the symptom
disappears after committing the bb->count... that would still be latent
somewhere.

thanks

Christian


> 
> Ciao!
> Steven
>

Patch

Index: tree-ssa-tail-merge.c
===================================================================
--- tree-ssa-tail-merge.c	(revision 191129)
+++ tree-ssa-tail-merge.c	(working copy)
@@ -1478,6 +1478,8 @@ 
     bb2->frequency = BB_FREQ_MAX;
   bb1->frequency = 0;
 
+  bb2->count += bb1->count;
+
   /* Do updates that use bb1, before deleting bb1.  */
   release_last_vdef (bb1);
   same_succ_flush_bb (bb1);