[3/3] Fix dbr_schedule for -freorder-blocks-and-partition

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.

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

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

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);
>

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