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

#2, #4 and #8 look good and can be applied if they work independently of the

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

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.


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.


Reply via email to