Patchwork shrink-wrapping duplicates BBs across partitions.

login
register
mail settings
Submitter Christian Bruel
Date Sept. 11, 2012, 12:25 p.m.
Message ID <504F2DA8.3050208@st.com>
Download mbox | patch
Permalink /patch/183087/
State New
Headers show

Comments

Christian Bruel - Sept. 11, 2012, 12:25 p.m.
Hello,

While testing the patch to enable shrink-wrapping on SH [PR54546], we
hit an the "error: EDGE_CROSSING missing across section boundary"

Indeed, shrink-wrap duplicates a bb with successors (containing the
return sequence) into an unlikely section. I first thought about setting
the EDGE_CROSSING on flag on those edge, but I feel that this block
duplication doesn't go in the direction of this optimization. Not
duplicating BBs between partitions solves the problem.

Does this restriction look right to you ? (regression tests are still
running on x86 and sh)

Thanks a lot for any comment.

Christian
Steven Bosscher - Sept. 11, 2012, 12:46 p.m.
> Does this restriction look right to you ? (regression tests are still
> running on x86 and sh)

Please generate your patches with diff -up (or svn diff -x -up).

> +		&& (BB_PARTITION (e->src) == BB_PARTITION (e->dest))

No need for parentheses around this check.

The shrink wrapping code appears to be dealing with partitioning, or
at least there are BB_COPY_PARTITIONs further down. So I can't tell
whether this fix is correct. Can you show in more detail what happens?
(A dotty graph is always helpful ;-)

Ciao!
Steven
Christian Bruel - Sept. 11, 2012, 3:31 p.m.
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.

My hypothesis, is that with a gcov based profile, we should never have
such partitioning on the edges, BB10 should be COLD as well. My
suggestion was to avoid shrink-wrapping failing on the block duplication
for this case, but that would hide the real cause. I now prefer to
understand why BB10 is HOT in the first place... if this is a correct
assumption that it should not be.

Thanks

Christian


On 09/11/2012 02:46 PM, Steven Bosscher wrote:
>> Does this restriction look right to you ? (regression tests are still
>> running on x86 and sh)
> 
> Please generate your patches with diff -up (or svn diff -x -up).
> 
>> +		&& (BB_PARTITION (e->src) == BB_PARTITION (e->dest))
> 
> No need for parentheses around this check.
> 
> The shrink wrapping code appears to be dealing with partitioning, or
> at least there are BB_COPY_PARTITIONs further down. So I can't tell
> whether this fix is correct. Can you show in more detail what happens?
> (A dotty graph is always helpful ;-)
> 
> Ciao!
> Steven
>
Steven Bosscher - Sept. 11, 2012, 3:40 p.m.
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.

Ciao!
Steven
Christian Bruel - Sept. 11, 2012, 3:46 p.m.
On 09/11/2012 05:40 PM, 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?

Thanks for the confirmation. The case happens on SH only when applying
the simple_return patch [PR target/54546] on the bb-reorder test from
the testsuite.

> It'd be good to have a PR for this.

I'll update the PR above with what I find, lets see if this turns out to
be target independent.

thanks

Christian

> 
> Ciao!
> Steven
>
Christian Bruel - Sept. 11, 2012, 4:17 p.m.
when running a cfg dump, I get many messages like:

Invalid sum of incoming frequencies 1667, should be 3334

So it looks like a profile information was not correctly propagated
somewhere. which could lead to such partitioning incoherency. I have no
idea for the moment if this is local problem or not, just want to share
that in case someone as an input on this.

Cheers

Christian


On 09/11/2012 05:40 PM, 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.
> 
> Ciao!
> Steven
>
Jakub Jelinek - Sept. 11, 2012, 4:21 p.m.
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

Patch

Index: function.c
===================================================================
--- function.c	(revision 191177)
+++ function.c	(working copy)
@@ -6063,6 +6063,7 @@ 
 	  FOR_EACH_EDGE (e, ei, tmp_bb->preds)
 	    if (single_succ_p (e->src)
 		&& !bitmap_bit_p (&bb_on_list, e->src->index)
+		&& (BB_PARTITION (e->src) == BB_PARTITION (e->dest))
 		&& can_duplicate_block_p (e->src))
 	      {
 		edge pe;