diff mbox

shrink-wrap: Once more PRs 67778, 68634, and now 68909

Message ID caa43fac2ad48df0eb694edebb60847a29a5763a.1450385490.git.segher@kernel.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool Dec. 17, 2015, 9:07 p.m. UTC 
It turns out v4 wasn't quite complete anyway; so here "v5".

If a candidate PRE cannot get the prologue because a block BB is
reachable from it, but PRE does not dominate BB, we try again with the
dominators of PRE.  That "try again" needs to again consider BB though,
we aren't done with it.

This fixes this problem.  Tested on the 68909 testcase, and bootstrapped
and regression checked on powerpc64-linux.  Is this okay for trunk?


Segher


2015-12-17  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/67778
	PR rtl-optimization/68634
	PR rtl-optimization/68909
	* shrink-wrap.c (try_shrink_wrapping): If BB isn't dominated by PRE,
	push it back on VEC.

---
 gcc/shrink-wrap.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Bernd Schmidt Dec. 18, 2015, 1:19 a.m. UTC | #1 
On 12/17/2015 10:07 PM, Segher Boessenkool wrote:
> It turns out v4 wasn't quite complete anyway; so here "v5".
>
> If a candidate PRE cannot get the prologue because a block BB is
> reachable from it, but PRE does not dominate BB, we try again with the
> dominators of PRE.  That "try again" needs to again consider BB though,
> we aren't done with it.
>
> This fixes this problem.  Tested on the 68909 testcase, and bootstrapped
> and regression checked on powerpc64-linux.  Is this okay for trunk?

This code is getting really quite confusing, and at the least I think we 
need more documentation of what exactly vec is supposed to contain at 
the entry to the inner while loop here.
I'm also beginning to think we should disable this part of the code for 
gcc-6.


Bernd
Segher Boessenkool Dec. 20, 2015, 4:27 p.m. UTC | #2 
On Fri, Dec 18, 2015 at 02:19:37AM +0100, Bernd Schmidt wrote:
> On 12/17/2015 10:07 PM, Segher Boessenkool wrote:
> >It turns out v4 wasn't quite complete anyway; so here "v5".
> >
> >If a candidate PRE cannot get the prologue because a block BB is
> >reachable from it, but PRE does not dominate BB, we try again with the
> >dominators of PRE.  That "try again" needs to again consider BB though,
> >we aren't done with it.
> >
> >This fixes this problem.  Tested on the 68909 testcase, and bootstrapped
> >and regression checked on powerpc64-linux.  Is this okay for trunk?
> 
> This code is getting really quite confusing,

Yes :-(  I don't think stage 3 is the time to completely rewrite it though.

> and at the least I think we 
> need more documentation of what exactly vec is supposed to contain at 
> the entry to the inner while loop here.

Same as in the other loop: vec is a stack of blocks that still need to
be looked at.  I can duplicate the comment if you want?

> I'm also beginning to think we should disable this part of the code for 
> gcc-6.

That would be a regression (from GCC 5); but I understand your worry.
How about we disable it if any further problems show up?


Segher
Bernd Schmidt Jan. 4, 2016, 2:24 p.m. UTC | #3 
On 12/20/2015 05:27 PM, Segher Boessenkool wrote:
> On Fri, Dec 18, 2015 at 02:19:37AM +0100, Bernd Schmidt wrote:
>> On 12/17/2015 10:07 PM, Segher Boessenkool wrote:
>>> It turns out v4 wasn't quite complete anyway; so here "v5".
>>>
>>> If a candidate PRE cannot get the prologue because a block BB is
>>> reachable from it, but PRE does not dominate BB, we try again with the
>>> dominators of PRE.  That "try again" needs to again consider BB though,
>>> we aren't done with it.
>>>
>>> This fixes this problem.  Tested on the 68909 testcase, and bootstrapped
>>> and regression checked on powerpc64-linux.  Is this okay for trunk?
>>
>> This code is getting really quite confusing,
>> and at the least I think we
>> need more documentation of what exactly vec is supposed to contain at
>> the entry to the inner while loop here.
>
> Same as in the other loop: vec is a stack of blocks that still need to
> be looked at.  I can duplicate the comment if you want?

No, I think more is needed. The inner loop looks like it should be 
emptying the vec, but this is not true if we break out of it, and your 
patch now even adds an explicit push. It also looks like it wants to use 
the bb_tmp bitmap to cache results for future iterations of the outer 
loop, but I'm not convinced this is actually correct. I can't follow 
this behaviour anymore without clear a description of intent.

Also, it might be clearer to not modify "pro" in this loop - use a 
"cand" variable, and modify "pro" instead of last_ok, getting rid of the 
latter.

> That would be a regression (from GCC 5); but I understand your worry.
> How about we disable it if any further problems show up?

Let's see whether we can make sense of this code and decide then.


bernd
diff mbox

Patch

diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
index f65b0c3..85e5a8b 100644
--- a/gcc/shrink-wrap.c
+++ b/gcc/shrink-wrap.c
@@ -781,6 +781,7 @@  try_shrink_wrapping (edge *entry_edge, bitmap_head *bb_with,
 	      if (!dominated_by_p (CDI_DOMINATORS, bb, pre))
 		{
 		  ok = false;
+		  vec.quick_push (bb);
 		  break;
 		}