* [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition
@ 2015-01-27 3:07 Kaz Kojima
2015-01-29 0:27 ` Jeff Law
2015-03-24 8:49 ` Steven Bosscher
0 siblings, 2 replies; 5+ messages in thread
From: Kaz Kojima @ 2015-01-27 3:07 UTC (permalink / raw)
To: gcc-patches
This patch is to fix 2 issues found in dbr_schedule when trying to
fix PR target/64761. The first is relax_delay_slots removes
the jump insn in the insns like below:
(jump_insn/j 74 58 59 (set (pc) (label_ref:SI 29)) ...)
(barrier 59 74 105)
(note 105 59 29 NOTE_INSN_SWITCH_TEXT_SECTIONS)
(code_label 29 105 30 31 "" [5 uses])
(insn 31 30 32 (set (reg ...
i.e. relax_delay_slot tries to delete the jump insn pointing to
the next active insn of that jump insn as a trivial jump even when
there is a NOTE_INSN_SWITCH_TEXT_SECTIONS note between that jump
and its next active insn.
The second issue is that relax_delay_slots does a variant of
follow jump optimization without checking targetm.can_follow_jump.
--
PR target/64761
* reorg.c (switch_text_sections_between_p): New function.
(relax_delay_slots): Call it when testing if the jump insn
is removable. Use targetm.can_follow_jump when testing if
the conditional branch can follow an unconditional jump.
diff --git a/reorg.c b/reorg.c
index 326fa53..2387910 100644
--- a/reorg.c
+++ b/reorg.c
@@ -3211,6 +3211,19 @@ label_before_next_insn (rtx x, rtx scan_limit)
return insn;
}
+/* Return TRUE if there is a NOTE_INSN_SWITCH_TEXT_SECTIONS note in between
+ BEG and END. */
+
+static bool
+switch_text_sections_between_p (const rtx_insn *beg, const rtx_insn *end)
+{
+ const rtx_insn *p;
+ for (p = beg; p != end; p = NEXT_INSN (p))
+ if (NOTE_P (p) && NOTE_KIND (p) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
+ return true;
+ return false;
+}
+
\f
/* Once we have tried two ways to fill a delay slot, make a pass over the
code to try to improve the results and to do such things as more jump
@@ -3247,7 +3260,8 @@ relax_delay_slots (rtx_insn *first)
target_label = find_end_label (target_label);
if (target_label && next_active_insn (target_label) == next
- && ! condjump_in_parallel_p (insn))
+ && ! condjump_in_parallel_p (insn)
+ && ! (next && switch_text_sections_between_p (insn, next)))
{
delete_jump (insn);
continue;
@@ -3262,12 +3276,13 @@ relax_delay_slots (rtx_insn *first)
/* See if this jump conditionally branches around an unconditional
jump. If so, invert this jump and point it to the target of the
- second jump. */
+ second jump. Check if it's possible on the target. */
if (next && simplejump_or_return_p (next)
&& any_condjump_p (insn)
&& target_label
&& next_active_insn (target_label) == next_active_insn (next)
- && no_labels_between_p (insn, next))
+ && no_labels_between_p (insn, next)
+ && targetm.can_follow_jump (insn, next))
{
rtx label = JUMP_LABEL (next);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition
2015-01-27 3:07 [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition Kaz Kojima
@ 2015-01-29 0:27 ` Jeff Law
2015-01-29 10:27 ` Kaz Kojima
2015-03-24 8:49 ` Steven Bosscher
1 sibling, 1 reply; 5+ messages in thread
From: Jeff Law @ 2015-01-29 0:27 UTC (permalink / raw)
To: Kaz Kojima, gcc-patches
On 01/26/15 16:52, Kaz Kojima wrote:
> This patch is to fix 2 issues found in dbr_schedule when trying to
> fix PR target/64761. The first is relax_delay_slots removes
> the jump insn in the insns like below:
>
> (jump_insn/j 74 58 59 (set (pc) (label_ref:SI 29)) ...)
> (barrier 59 74 105)
> (note 105 59 29 NOTE_INSN_SWITCH_TEXT_SECTIONS)
> (code_label 29 105 30 31 "" [5 uses])
> (insn 31 30 32 (set (reg ...
>
> i.e. relax_delay_slot tries to delete the jump insn pointing to
> the next active insn of that jump insn as a trivial jump even when
> there is a NOTE_INSN_SWITCH_TEXT_SECTIONS note between that jump
> and its next active insn.
> The second issue is that relax_delay_slots does a variant of
> follow jump optimization without checking targetm.can_follow_jump.
>
> --
> PR target/64761
> * reorg.c (switch_text_sections_between_p): New function.
> (relax_delay_slots): Call it when testing if the jump insn
> is removable. Use targetm.can_follow_jump when testing if
> the conditional branch can follow an unconditional jump.
OK. I'm a bit surprised we're still finding this kind of stuff 10 years
after NOTE_INSN_SWITCH_TEXT_SECTIONS went in. Sigh.
Jeff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition
2015-01-29 0:27 ` Jeff Law
@ 2015-01-29 10:27 ` Kaz Kojima
0 siblings, 0 replies; 5+ messages in thread
From: Kaz Kojima @ 2015-01-29 10:27 UTC (permalink / raw)
To: law; +Cc: gcc-patches
Jeff Law <law@redhat.com> wrote:
> OK. I'm a bit surprised we're still finding this kind of stuff 10
> years after NOTE_INSN_SWITCH_TEXT_SECTIONS went in. Sigh.
Thanks for reviewing. I've committed it as revision 220235
after testing the patch independently.
Regards,
kaz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition
2015-01-27 3:07 [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition Kaz Kojima
2015-01-29 0:27 ` Jeff Law
@ 2015-03-24 8:49 ` Steven Bosscher
2015-03-24 13:20 ` Kaz Kojima
1 sibling, 1 reply; 5+ messages in thread
From: Steven Bosscher @ 2015-03-24 8:49 UTC (permalink / raw)
To: Kaz Kojima; +Cc: GCC Patches
On Tue, Jan 27, 2015 at 12:52 AM, Kaz Kojima wrote:
> This patch is to fix 2 issues found in dbr_schedule when trying to
> fix PR target/64761. The first is relax_delay_slots removes
> the jump insn in the insns like below:
>
> (jump_insn/j 74 58 59 (set (pc) (label_ref:SI 29)) ...)
> (barrier 59 74 105)
> (note 105 59 29 NOTE_INSN_SWITCH_TEXT_SECTIONS)
> (code_label 29 105 30 31 "" [5 uses])
> (insn 31 30 32 (set (reg ...
>
> i.e. relax_delay_slot tries to delete the jump insn pointing to
> the next active insn of that jump insn as a trivial jump even when
> there is a NOTE_INSN_SWITCH_TEXT_SECTIONS note between that jump
> and its next active insn.
> The second issue is that relax_delay_slots does a variant of
> follow jump optimization without checking targetm.can_follow_jump.
>
> --
> PR target/64761
> * reorg.c (switch_text_sections_between_p): New function.
> (relax_delay_slots): Call it when testing if the jump insn
> is removable. Use targetm.can_follow_jump when testing if
> the conditional branch can follow an unconditional jump.
This patch merely papers over another issue, probably a missing
CROSSING_JUMP_P test.
Ciao!
Steven
> diff --git a/reorg.c b/reorg.c
> index 326fa53..2387910 100644
> --- a/reorg.c
> +++ b/reorg.c
> @@ -3211,6 +3211,19 @@ label_before_next_insn (rtx x, rtx scan_limit)
> return insn;
> }
>
> +/* Return TRUE if there is a NOTE_INSN_SWITCH_TEXT_SECTIONS note in between
> + BEG and END. */
> +
> +static bool
> +switch_text_sections_between_p (const rtx_insn *beg, const rtx_insn *end)
> +{
> + const rtx_insn *p;
> + for (p = beg; p != end; p = NEXT_INSN (p))
> + if (NOTE_P (p) && NOTE_KIND (p) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
> + return true;
> + return false;
> +}
> +
>
> /* Once we have tried two ways to fill a delay slot, make a pass over the
> code to try to improve the results and to do such things as more jump
> @@ -3247,7 +3260,8 @@ relax_delay_slots (rtx_insn *first)
> target_label = find_end_label (target_label);
>
> if (target_label && next_active_insn (target_label) == next
> - && ! condjump_in_parallel_p (insn))
> + && ! condjump_in_parallel_p (insn)
> + && ! (next && switch_text_sections_between_p (insn, next)))
> {
> delete_jump (insn);
> continue;
> @@ -3262,12 +3276,13 @@ relax_delay_slots (rtx_insn *first)
>
> /* See if this jump conditionally branches around an unconditional
> jump. If so, invert this jump and point it to the target of the
> - second jump. */
> + second jump. Check if it's possible on the target. */
> if (next && simplejump_or_return_p (next)
> && any_condjump_p (insn)
> && target_label
> && next_active_insn (target_label) == next_active_insn (next)
> - && no_labels_between_p (insn, next))
> + && no_labels_between_p (insn, next)
> + && targetm.can_follow_jump (insn, next))
> {
> rtx label = JUMP_LABEL (next);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition
2015-03-24 8:49 ` Steven Bosscher
@ 2015-03-24 13:20 ` Kaz Kojima
0 siblings, 0 replies; 5+ messages in thread
From: Kaz Kojima @ 2015-03-24 13:20 UTC (permalink / raw)
To: stevenb.gcc; +Cc: gcc-patches
Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> This patch merely papers over another issue, probably a missing
> CROSSING_JUMP_P test.
Perhaps. Surely it has looked the current DBR is not so well for
crossing jumps and my fix might be a bit ad-hoc.
The first part of the patch could be rewritten with checking if one
of the following jumps is crossing, though I thought that checking
NOTE_INSN_SWITCH_TEXT_SECTIONS is straight forward to see the jump
is trivial or not.
Regards,
kaz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-03-24 13:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 3:07 [PATCH 3/3] Fix dbr_schedule for -freorder-blocks-and-partition Kaz Kojima
2015-01-29 0:27 ` Jeff Law
2015-01-29 10:27 ` Kaz Kojima
2015-03-24 8:49 ` Steven Bosscher
2015-03-24 13:20 ` Kaz Kojima
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).