Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On 09/20/2016 11:11 AM, Segher Boessenkool wrote:
On Tue, Sep 20, 2016 at 08:52:46AM -0600, Jeff Law wrote:
- 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)).
Yes. But rtl.texi still says:
Return insns count as jumps, but since they do not refer to any
labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.
And the comment at the top of jump.c:
Each CODE_LABEL has a count of the times it is used
stored in the LABEL_NUSES internal field, and each JUMP_INSN
has one label that it refers to stored in the
JUMP_LABEL internal field. With this we can detect labels that
become unused because of the deletion of all the jumps that
formerly used them. The JUMP_LABEL info is sometimes looked
at by later passes. For return insns, it contains either a
RETURN or a SIMPLE_RETURN rtx.
It's intentional, and really there's nothing wrong with it, or with
operands[] containing CODE_LABELs. The problem is trying to force a
I'm just pointing out the documentation is out-of-date here. I'll do
a patch.
static type system onto a dynamically typed data structure.
But what this doesn't answer is why we stuff RETURN/SIMPLE_RETURN into
the JUMP_LABEL field of a return. I simply can't remember any rationale
behind that.
It is very inconvenient to parse the whole rtx to see if this is a
RETURN vs. a SIMPLE_RETURN (it can be inside a conditional or inside
a PARALLEL, etc.)
So it's similar to how we use JUMP_LABEL to find the jump target without
having to dig through the whole rtx.
Ideally we would not have RETURN at all anymore, just SIMPLE_RETURN,
but that isn't going to happen any time soon.
And it wouldn't totally resolve this anyway.
I suspect that if we dig further, we'll find that we can accomplish
whatever the goal was behind that decision in a way that easily works in
a world where we have separated rtx objects from objects that are part
of the insn chain.
We cannot easily extract the JUMP_LABEL of a normal jump from its rtx
pattern either (if at all). So it seems the extra field of a JUMP_INSN
is here to stay. We'll have to figure out how to nicely do INSN vs.
JUMP_INSN in an rtx_insn world.
Both INSN and JUMP_INSNs are rtx_insn *s.
The problem is we're stuffing a (return) or (simple_return) rtx into a
field that most of the time has an rtx_insn * (CODE_LABEL).
A hack-ish way to address this would be to create special CODE_LABELs
that represent (return) and (simple_return) and stuff that into the
JUMP_LABEL field. At which point the JUMP_LABEL field should turn into
an rtx_insn * and the as_a casts related to this wart go away.
There may be cleaner ways, but there's certainly ways to move forward here.
Just to be clear, I don't see us going to a world where everything we
have as an rtx today is a statically typed object. But there are things
that I think make sense to carve out, the most obvious being objects
which are part of the insn chain.
Agreed on both. I have nightmares about the first, so please don't even
talk about going there :-)
While I could see some value in static typing enough to allow static
checking things like we don't try to access out-of-range operands (say
XEXP (x, 1)) where X is a unary code. I don't see the level of pain to
get there being worth it.
It's much like what we see in the tree world. There's value in
separating _DECL, _TYPE and _EXPR nodes. But going deeper into in the
_EXPR nodes seems like a huge level of pain.
jeff
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On Tue, Sep 20, 2016 at 08:52:46AM -0600, Jeff Law wrote:
> >- 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)).
> >>
> >>Yes. But rtl.texi still says:
> >>
> >> Return insns count as jumps, but since they do not refer to any
> >> labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.
> >
> >And the comment at the top of jump.c:
> >
> > Each CODE_LABEL has a count of the times it is used
> > stored in the LABEL_NUSES internal field, and each JUMP_INSN
> > has one label that it refers to stored in the
> > JUMP_LABEL internal field. With this we can detect labels that
> > become unused because of the deletion of all the jumps that
> > formerly used them. The JUMP_LABEL info is sometimes looked
> > at by later passes. For return insns, it contains either a
> > RETURN or a SIMPLE_RETURN rtx.
> >
> >It's intentional, and really there's nothing wrong with it, or with
> >operands[] containing CODE_LABELs. The problem is trying to force a
I'm just pointing out the documentation is out-of-date here. I'll do
a patch.
> >static type system onto a dynamically typed data structure.
> But what this doesn't answer is why we stuff RETURN/SIMPLE_RETURN into
> the JUMP_LABEL field of a return. I simply can't remember any rationale
> behind that.
It is very inconvenient to parse the whole rtx to see if this is a
RETURN vs. a SIMPLE_RETURN (it can be inside a conditional or inside
a PARALLEL, etc.)
Ideally we would not have RETURN at all anymore, just SIMPLE_RETURN,
but that isn't going to happen any time soon.
> I suspect that if we dig further, we'll find that we can accomplish
> whatever the goal was behind that decision in a way that easily works in
> a world where we have separated rtx objects from objects that are part
> of the insn chain.
We cannot easily extract the JUMP_LABEL of a normal jump from its rtx
pattern either (if at all). So it seems the extra field of a JUMP_INSN
is here to stay. We'll have to figure out how to nicely do INSN vs.
JUMP_INSN in an rtx_insn world.
> Just to be clear, I don't see us going to a world where everything we
> have as an rtx today is a statically typed object. But there are things
> that I think make sense to carve out, the most obvious being objects
> which are part of the insn chain.
Agreed on both. I have nightmares about the first, so please don't even
talk about going there :-)
Segher
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On 09/20/2016 04:38 AM, Bernd Schmidt wrote:
On 09/20/2016 01:16 AM, Segher Boessenkool wrote:
On Mon, Sep 19, 2016 at 06:07:46PM -0400, Trevor Saunders wrote:
- 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)).
Yes. But rtl.texi still says:
Return insns count as jumps, but since they do not refer to any
labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.
And the comment at the top of jump.c:
Each CODE_LABEL has a count of the times it is used
stored in the LABEL_NUSES internal field, and each JUMP_INSN
has one label that it refers to stored in the
JUMP_LABEL internal field. With this we can detect labels that
become unused because of the deletion of all the jumps that
formerly used them. The JUMP_LABEL info is sometimes looked
at by later passes. For return insns, it contains either a
RETURN or a SIMPLE_RETURN rtx.
It's intentional, and really there's nothing wrong with it, or with
operands[] containing CODE_LABELs. The problem is trying to force a
static type system onto a dynamically typed data structure.
But what this doesn't answer is why we stuff RETURN/SIMPLE_RETURN into
the JUMP_LABEL field of a return. I simply can't remember any rationale
behind that.
I suspect that if we dig further, we'll find that we can accomplish
whatever the goal was behind that decision in a way that easily works in
a world where we have separated rtx objects from objects that are part
of the insn chain.
Just to be clear, I don't see us going to a world where everything we
have as an rtx today is a statically typed object. But there are things
that I think make sense to carve out, the most obvious being objects
which are part of the insn chain.
jeff
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On 09/20/2016 01:16 AM, Segher Boessenkool wrote:
On Mon, Sep 19, 2016 at 06:07:46PM -0400, Trevor Saunders wrote:
- 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)).
Yes. But rtl.texi still says:
Return insns count as jumps, but since they do not refer to any
labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.
And the comment at the top of jump.c:
Each CODE_LABEL has a count of the times it is used
stored in the LABEL_NUSES internal field, and each JUMP_INSN
has one label that it refers to stored in the
JUMP_LABEL internal field. With this we can detect labels that
become unused because of the deletion of all the jumps that
formerly used them. The JUMP_LABEL info is sometimes looked
at by later passes. For return insns, it contains either a
RETURN or a SIMPLE_RETURN rtx.
It's intentional, and really there's nothing wrong with it, or with
operands[] containing CODE_LABELs. The problem is trying to force a
static type system onto a dynamically typed data structure.
Bernd
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On Mon, Sep 19, 2016 at 06:07:46PM -0400, Trevor Saunders wrote:
> > > - 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)).
Yes. But rtl.texi still says:
Return insns count as jumps, but since they do not refer to any
labels, their @code{JUMP_LABEL} is @code{NULL_RTX}.
Segher
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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+...@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
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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 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
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On 09/16/2016 04:10 AM, Bernd Schmidt wrote: On 09/16/2016 12:10 PM, Trevor Saunders wrote: 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. - JUMP_LABEL can be a return which is not an insn. That also seems like something that should be improved at some point. These just show that fundamentally, rtl is just dynamically typed, and used as such, which is why I was never massively enthusiastic about the rtx -> rtx_insn conversion to begin with. RTL is dynamically typed, but I think we can start carving off things like the insn chain objects -- they don't need to be dynamically typed. Essentially in my mind the insn chain doesn't need to be RTL. It's really an RTL container. It does mean there are things we have to fix up, but so far the things we've fixed up, I've considered clear improvements. Jeff
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On Fri, Sep 16, 2016 at 12:10:51PM +0200, Bernd Schmidt wrote: > On 09/16/2016 12:10 PM, Trevor Saunders wrote: > > 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. > > > > - JUMP_LABEL can be a return which is not an insn. That also seems > > like something that should be improved at some point. > > These just show that fundamentally, rtl is just dynamically typed, and used > as such, which is why I was never massively enthusiastic about the rtx -> > rtx_insn conversion to begin with. I would agree that rtl is dynamically typed at the moment, and viewing rtx_insn as an attempt to change that is certainly reasonable. I don't know that any of these things mean rtl has to be dynamically typed. I think that effort has already helped some things, I wouldn't want to think about trying to get rid of rtx_insn_list without it. Further I expect it will make it possible to change the data structures here more, and I suspect there is room for cleverness there that isn't possible with dynamic typing. Changing away from dynamic typing ccertainly isn't easy, but I think there's a good amount of evidence it isn't well suited to large complicated projects. Alternatively if you take the view that dynamic typing is just a special case of static typing with one type then adding more types allows you to reduce the number of places you need to check the "type" of an object. If we can enable some of what rtl checking gets us with out the compile time penalty that certainly seems valuable. Trev > > > Bernd
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
On 09/16/2016 12:10 PM, Trevor Saunders wrote: 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. - JUMP_LABEL can be a return which is not an insn. That also seems like something that should be improved at some point. These just show that fundamentally, rtl is just dynamically typed, and used as such, which is why I was never massively enthusiastic about the rtx -> rtx_insn conversion to begin with. Bernd
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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 > > > > 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. - JUMP_LABEL can be a return which is not an insn. That also seems like something that should be improved at some point. - tablejump_p returns a label through a rtx * out argument. I expect that isn't hard to fix at this point. - 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. - var_loc_node::loc is either an expr list or a note, off hand I'm not sure what to do with this. - 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. Trev > jeff
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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 > > > > 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. Well, this series does remove a few, though I'm sure it is a net addition. > And that might be a good way to prioritize the next batch of work. Pick a > cast, remove it and deal with the fallout. :-) yeah, that's more or less what I did here. each of the next_X_insn methods took a rtx and cast it to rtx_insn *, so I changed it to take a rtx_insn * and dropped the cast. Then I fixed up the fallout and moved the conversion of variable types to the beginning of the series since it is less objectionable. > > > > > 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! heh, that'd be useful. Though I'm not really sure how important it is to clean up code specific to targets with delay slots. Trev > jeff
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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 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! jeff
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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 > > 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. > 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. Trev > > > Bernd
Re: [PATCH 0/8] make next_*_insn take rtx_insn * arguments
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 cast. #2, #4 and #8 look good and can be applied if they work independently of the others. Less certain about some of the others which introduce additional casts. 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. Bernd
