Patchwork [RFA,2/n] Don't lift loads above register using jumps in postreload-gcse.c

login
register
mail settings
Submitter Matthew Gretton-Dann
Date Sept. 25, 2012, 7:12 p.m.
Message ID <3828314.5fHR3eBOVy@e103209-lin>
Download mbox | patch
Permalink /patch/186880/
State New
Headers show

Comments

Matthew Gretton-Dann - Sept. 25, 2012, 7:12 p.m.
On Wednesday 05 September 2012 17:40:23 Steven Bosscher wrote:
> On Wed, Sep 5, 2012 at 3:18 PM, Matthew Gretton-Dann
> 
> <matthew.gretton-dann@linaro.org> wrote:
> > On 5 September 2012 13:45, Richard Earnshaw <rearnsha@arm.com> wrote:
> >> On 05/09/12 13:02, Steven Bosscher wrote:
> >>> On Wed, Sep 5, 2012 at 1:42 PM, Matthew Gretton-Dann wrote:
> >>>> Whilst this fix works for this particular case I am not sure it is the
> >>>> best fix for the general issue, and so if others have a better idea how
> >>>> to fix this I would be very happy.
> >>> 
> >>> postreload-gcse.c is broken in "interesting" ways. Look at this gem for
> >>> example:
> >>> 
> >>> static bool
> >>> reg_changed_after_insn_p (rtx x, int cuid)
> >>> {
> >>> 
> >>>   unsigned int regno, end_regno;
> >>>   
> >>>   regno = REGNO (x);
> >>>   end_regno = END_HARD_REGNO (x);
> >>>   do
> >>>   
> >>>     if (reg_avail_info[regno] > cuid)
> >>>     
> >>>       return true;
> >>>   
> >>>   while (++regno < end_regno);
> >>>   return false;
> >>> 
> >>> }
> >>> 
> >>> So the more conservative the fix, the better :-)
> > 
> > I suppose removing the pass is too conservative :-)
> > 
> >>> The patch looks correct to me. But perhaps the pass should just punt
> >>> on blocks not ending in a simple jump in
> >>> bb_has_well_behaved_predecessors?
> > 
> > By 'simple jump' you mean any block with at most only EDGE_FALLTHRU on the
> > edge?
> No, I mean using the onlyjump_p predicate.

Again sorry for the delay.  Attached is an updated patch using the onlyjump_p 
predicate as suggested by Steven.

Tested cross arm-none-linux-gnueabi with QEmu.

OK for trunk?

Thanks,

Matt

gcc/ChangeLog:

2012-09-25  Matthew Gretton-Dann  <matthew.gretton-dann@arm.com>

	* postreload-gcse.c (bb_has_well_behaved_predecessors): Don't handle blocks
	that end in a non-simple jump.
Steven Bosscher - Sept. 25, 2012, 8:19 p.m.
On Tue, Sep 25, 2012 at 9:12 PM, Matthew Gretton-Dann
<matthew.gretton-dann@linaro.org> wrote:
>> No, I mean using the onlyjump_p predicate.
>
> Again sorry for the delay.  Attached is an updated patch using the onlyjump_p
> predicate as suggested by Steven.

+      if (onlyjump_p (BB_END (pred->src)))

Eh, don't you want (!onlyjump_p (BB_END (pred->src))) ?
Note the "not".

You also have to deal with non-jump BB_END insns.

Ciao!
Steven

Patch

diff --git a/gcc/postreload-gcse.c b/gcc/postreload-gcse.c
index b9e9f25..412c8fc 100644
--- a/gcc/postreload-gcse.c
+++ b/gcc/postreload-gcse.c
@@ -925,6 +925,9 @@  bb_has_well_behaved_predecessors (basic_block bb)
 
       if (JUMP_TABLE_DATA_P (BB_END (pred->src)))
 	return false;
+
+      if (onlyjump_p (BB_END (pred->src)))
+	return false;
     }
   return true;
 }