diff mbox

shrink-wrapping duplicates BBs across partitions.

Message ID 504F2DA8.3050208@st.com
State New
Headers show

Commit Message

Christian Bruel Sept. 11, 2012, 12:25 p.m. UTC 
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

Comments

Steven Bosscher Sept. 11, 2012, 12:46 p.m. UTC | #1 
> 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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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. UTC | #5 
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. UTC | #6 
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
diff mbox

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;