Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments

[Jeff Law <law at redhat dot com>]

On 09/16/2016 04:10 AM, Trevor Saunders wrote:
> On Thu, Sep 15, 2016 at 10:27:56AM -0600, Jeff Law wrote:
> > On 09/15/2016 10:10 AM, Trevor Saunders wrote:
> > > On Thu, Sep 15, 2016 at 12:52:59PM +0200, Bernd Schmidt wrote:
> > > > On 09/14/2016 09:21 PM, tbsaunde+gcc@tbsaunde.org wrote:
> > > >
> > > > > Basically $subject.  First change variable's type to rtx_insn * where possible.
> > > > > Then change the functions and fixup callers where it is still necessary to
> > > > > cast.
> > > >
> > > > #2, #4 and #8 look good and can be applied if they work independently of the
> > > > others.
> > >
> > > at most #2 should depend on #1 so it should be fine and I can check on
> > > the others.
> > >
> > > > Less certain about some of the others which introduce additional casts.
> > >
> > > yeah, its somewhat unfortunate, though one way or another we will need
> > > to add casts I think the question is just how many we will accept and
> > > where.
> > In my mind the casts represent the "bounds" of how far we've taken this
> > work.  They occur at the boundaries where we haven't converted something
> > from an "rtx" to an "rtx_insn *" and allow us to do the work piecemeal
> > rather than all-at-once.
> >
> > What I don't have a sense of is when we'll be able to push rtx_insn * far
> > enough that we're able to start removing casts.
> >
> > And that might be a good way to prioritize the next batch of work.  Pick a
> > cast, remove it and deal with the fallout.  :-)
> >
> >
> > > > Maybe LABEL_REF_LABEL needs converting first, and reorg.c has a few
> > > > variables that might have to be made rtx_insn * in patch #7 to avoid casts.
> > >
> > > LABEL_REF_LABEL might be doable, its a good idea I'll look into.  The
> > > reorg.c stuff around target_label is rather complicated unfortunately.
> > > In the end I of course agree the variables should be rtx_insn *.
> > > However currently things are assigned to that variable that are not
> > > insns.  So we need to break the variable up, but its involved in a lot
> > > of code I don't think I know well enough to really refactor.  For
> > > example it looks like target_label can hold a value between iterations
> > > of the loop, I suspect that would be a bug, but I'm not really sure.
> > I can probably help with reorg.  Hell, you might even be referring to my
> > code!
>
> ok, going through all the casts added here I see the following reasons
> for them.
>
> - in md files operands is a array of rtx, we should probably have a
>   different way to pass insns to the C code here.  That seems worth
>   investigating incrementally.
Agreed.  Presumably the exception is passing around something like a 
CODE_LABEL?   Or maybe some hackery with passing around a jump table?



> - JUMP_LABEL can be a return which is not an insn.  That also seems
>   like something that should be improved at some point.
The JUMP_LABEL field within a JUMP_INSN?  That sounds like a bug.


> - tablejump_p returns a label through a rtx * out argument.  I expect
>   that isn't hard to fix at this point.
Right.  Seems like a reasonable follow-up.

> - sh.c uses XEXP (SET_SRC (PATTERN (jump)) 0) with a comment JUMP_LABEL
>   might be undefined when not optimizing.  Its not clear to me if that
>   is still true.
The code in question is nearly 19 years old.  What I think Joern was 
referring to here was that in the old days jump.c was responsible for 
setting JUMP_LABEL and that would only happen when optimizing.

JUMP_LABEL was a convenient way to find the jump target of an insn.  It 
didn't matter where the label appeared as an operand.  It was also the 
case that we could derive a label, even though the insn might have been 
an indirect jump.

Anyway...

So instead of relying on JUMP_LABEL he extracts the destination out of 
the pattern (JUMP_LABEL is field outside the insn's pattern).  That 
destination should be a LABEL_REF.  Operand 0 of a LABEL_REF is its 
associated CODE_LABEL.  In the sh.c case, he happens to know precisely 
where the LABEL_REF will be inside the insn's pattern.

This points out one of the little hairballs we're going to want to sort 
out.  Essentially XEXP (X, 0) where X is a LABEL_REF needs to be an 
rtx_insn *.  Right now it's an rtx.

> - var_loc_node::loc is either an expr list or a note, off hand I'm not
>   sure what to do with this.
Depending on how it's set we may just want to break this into two fields.

> - in reorg.c variables refer to both a insn and other things I think
>   this is more results of JUMP_LABEL some times being a return.
I'd probably want to sit down with a debugger or with a more detailed 
description of when this happens.  I wouldn't be surprised if this was 
just a convenient way of marking jumps which we were later going to turn 
into simple returns.

> I've locally converted LABEL_REF_LABEL, but that only avoids 2 casts.
> However it does seem like an improvement so I'll send that shortly.
Sounds good.

jeff