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

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

On Mon, Sep 19, 2016 at 02:49:49PM -0600, Jeff Law wrote:
> 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?

 I took a quick sample of the casts in .md files.  It looks like in most
 if not all cases we have something like label_ref (match_operand 3 ""
 "") then we use operands[3] somewhere that expects a rtx_insn *.  So I
 guess it would make sense to add a match_label and add some magic to
 genrecog to populate a label_operands with the matched labels.

> > 
> > - 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.

yes, see the comment before the declaration of rtx_jump_insn::jump_label ()
it can be either a possibly deleted label, or a RETURN or SIMPLE_RETURN
expression.  Memory usage concerns aside its a tempting design to
change, but at the moment itts definitely intended to work this way,
there's plenty of code that checks ANY_RETURN_P (JUMP_LABEL (x)).

> > - 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.

yeah, I did it locally, so I just need to split out the parts of this
queue we can all agree should go in.

> > 
> > - 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.

yeah, I suspect the answer should be that the union has a field for
insns as well as for rtx, and we use XINSN instead of XEXP here.
Ultimmitly in any data structure that stores multiple things in one
place you need to cast somewhere.

> > 
> > - 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.

yeah, I haven't looked in depth yet.

> > 
> > - 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.

sure, unfortunately I'm not sure I have a simple example of when it
happens on hand.  However I've definitely seen it when forgeting aout
checking ANY_RETURN_P on the result of JUMP_LABEL, I may even have seen
it on x86 somewhere other than reorg.c.

Trev

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