On Mon, Sep 19, 2016 at 03:17:33PM -0600, Jeff Law wrote:
> On 09/14/2016 01:21 PM, tbsaunde+...@tbsaunde.org wrote:
> > From: Trevor Saunders <tbsaunde+...@tbsaunde.org>
> >
> > gcc/ChangeLog:
> >
> > 2016-09-14 Trevor Saunders <tbsaunde+...@tbsaunde.org>
> >
> > * emit-rtl.c (next_active_insn): Change argument type to
> > rtx_insn *.
> > (prev_active_insn): Likewise.
> > (active_insn_p): Likewise.
> > * rtl.h: Adjust prototypes.
> > * cfgcleanup.c (merge_blocks_move_successor_nojumps): Adjust.
> > * config/arc/arc.md: Likewise.
> > * config/pa/pa.c (branch_to_delay_slot_p): Likewise.
> > (branch_needs_nop_p): Likewise.
> > (use_skip_p): Likewise.
> > * config/sh/sh.c (gen_block_redirect): Likewise.
> > (split_branches): Likewise.
> > * reorg.c (optimize_skip): Likewise.
> > (fill_simple_delay_slots): Likewise.
> > (fill_slots_from_thread): Likewise.
> > (relax_delay_slots): Likewise.
> > * resource.c (mark_target_live_regs): Likewise.
> > ---
> > gcc/cfgcleanup.c | 2 +-
> > gcc/config/arc/arc.md | 33 +++++++++++++++++++++++----------
> > gcc/config/pa/pa.c | 6 +++---
> > gcc/config/sh/sh.c | 8 ++++----
> > gcc/emit-rtl.c | 10 +++-------
> > gcc/reorg.c | 29 +++++++++++++++++------------
> > gcc/resource.c | 2 +-
> > gcc/rtl.h | 6 +++---
> > 8 files changed, 55 insertions(+), 41 deletions(-)
> >
> > diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
> > index 023b9d2..2e2a635 100644
> > --- a/gcc/cfgcleanup.c
> > +++ b/gcc/cfgcleanup.c
> > @@ -708,7 +708,7 @@ merge_blocks_move_successor_nojumps (basic_block a,
> > basic_block b)
> > /* If there is a jump table following block B temporarily add the jump
> > table
> > to block B so that it will also be moved to the correct location. */
> > if (tablejump_p (BB_END (b), &label, &table)
> > - && prev_active_insn (label) == BB_END (b))
> > + && prev_active_insn (as_a<rtx_insn *> (label)) == BB_END (b))
> Looking at tablejump_p, there seems to be a belief that JUMP_LABEL could be
> a RETURN (as you mentioned in a latter message). That seems like something
> we should tackle, but I think that can happen independently.
>
> Presumably if we get a RETURN or SIMPLE_RETURN for LABEL the as_a will fail
> because it's not something that can be converted into an rtx_insn *.
tablejump_p checks for that and only returns an actually label. Which
is good because as_a would assert now in the caller, but previously it
would inside prev_active_insn.
>
>
> diff --git a/gcc/config/sh/sh.c b/gcc/config/sh/sh.c
> > index b3e949e..a9b5a14 100644
> > --- a/gcc/config/sh/sh.c
> > +++ b/gcc/config/sh/sh.c
> > @@ -5503,7 +5503,8 @@ gen_block_redirect (rtx_insn *jump, int addr, int
> > need_block)
> >
> > else if (optimize && need_block >= 0)
> > {
> > - rtx_insn *next = next_active_insn (next_active_insn (dest));
> > + rtx_insn *next = next_active_insn (as_a<rtx_insn *> (dest));
> It may be better to fix how we initialize "dest" to ensure it's an rtx_insn
> *. Essentially this:
>
> rtx dest = XEXP (SET_SRC (PATTERN (jump)), 0);
>
> turns into:
>
> rtx temp = XEXP (SET_SRC (PATTERN (jump)), 0);
> rtx_insn *dest = as_a<rtx_insn *> (temp));
>
> But presumably we can't do this until INSN_UID (and perhaps other stuff) is
> changed to accept an rtx_insn *.
actually that part is fine, rtx_insn * can implicitly convert to rtx,
the problem is a few lines down we assign JUMP_LABEL (next) to
dest, which could certainly be changed, but it seemed better to do the
simple thing for now.
> So that's future work.
>
>
> > @@ -2575,7 +2575,8 @@ fill_slots_from_thread (rtx_jump_insn *insn, rtx
> > condition,
> > to call update_block and delete_insn. */
> > fix_reg_dead_note (prior_insn, insn);
> > update_reg_unused_notes (prior_insn, new_thread);
> > - new_thread = next_active_insn (new_thread);
> > + new_thread
> > + = next_active_insn (as_a<rtx_insn *> (new_thread));
> Probably can't avoid the cast for now without other surgery. Nothing that
> should be terrible, but I don't think you need to pull on the that thread
> (pun intended) for this patch.
heh, that's my impression too.
>
> In fact, I'm generally OK with adding the casts in reorg.c until fairly late
> in this process :-)
>
> diff --git a/gcc/resource.c b/gcc/resource.c
> > index ae2f5d8..1d7ce95 100644
> > --- a/gcc/resource.c
> > +++ b/gcc/resource.c
> > @@ -1122,7 +1122,7 @@ mark_target_live_regs (rtx_insn *insns, rtx
> > target_maybe_return, struct resource
> > rtx_insn *stop_insn = next_active_insn (jump_insn);
> >
> > if (!ANY_RETURN_P (jump_target))
> > - jump_target = next_active_insn (jump_target);
> > + jump_target = next_active_insn (as_a<rtx_insn *> (jump_target));
> Right. jump_target here can only be an rtx_insn * because of controlling
> condition. But clearly this is something we'll want to clean up later.
presumably once we figure out what we want to do about JUMP_LABEL not
always being a label, and instead being a RETURN / SIMPLE_RETURN and do
that this will get cleaned up in the process.
Trev
>
> I think this is fine once prereqs are approved.
>
> jeff
