Message ID | 1721935.92zzXaUnD9@e103209-lin |
---|---|
State | New |
Headers | show |
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 :-) 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? Ciao! Steven
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 :-) > > 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? > > Ciao! > Steven > That sort of makes sense. Why would we ever want to hoist an insn out of a cold block into a hot one? I could see it making sense to do the reverse on occasion, but clearly care is needed here. R.
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? > That sort of makes sense. Why would we ever want to hoist an insn out > of a cold block into a hot one? I could see it making sense to do the > reverse on occasion, but clearly care is needed here. So whilst testing -freorder-blocks-and-partition has caused this behaviour to be exhibited, I believe there is nothing stopping this happening with any indirect jump - not just crossing jumps. Thanks, Matt
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. Ciao! Steven
diff --git a/gcc/postreload-gcse.c b/gcc/postreload-gcse.c index b464d1f..85fb9b3 100644 --- a/gcc/postreload-gcse.c +++ b/gcc/postreload-gcse.c @@ -1048,6 +1048,13 @@ eliminate_partially_redundant_load (basic_block bb, rtx /* Adding a load on a critical edge will cause a split. */ if (EDGE_CRITICAL_P (pred)) critical_edge_split = true; + + /* If the destination register is used at the BB end we can not + insert the load. */ + if (reg_used_between_p (dest, PREV_INSN (BB_END (pred_bb)), + next_pred_bb_end)) + goto cleanup; + not_ok_count += pred->count; unoccr = (struct unoccr *) obstack_alloc (&unoccr_obstack, sizeof (struct unoccr));