On 09/16/2016 04:10 AM, Trevor Saunders wrote:
Agreed. Presumably the exception is passing around something like a
CODE_LABEL? Or maybe some hackery with passing around a jump table?
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
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
- 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
- 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.
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.
- 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.
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.
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.
- 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'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.