This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 007/236] New function: for_each_rtx_in_insn
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: Jeff Law <law at redhat dot com>, gcc-patches at gcc dot gnu dot org
- Date: Thu, 14 Aug 2014 22:36:47 +0100
- Subject: Re: [PATCH 007/236] New function: for_each_rtx_in_insn
- Authentication-results: sourceware.org; auth=none
- References: <1407345815-14551-1-git-send-email-dmalcolm at redhat dot com> <1407345815-14551-8-git-send-email-dmalcolm at redhat dot com> <53EA8260 dot 3020501 at redhat dot com> <1408050295 dot 28418 dot 226 dot camel at surprise>
David Malcolm <dmalcolm@redhat.com> writes:
> On Tue, 2014-08-12 at 15:08 -0600, Jeff Law wrote:
>> On 08/06/14 11:19, David Malcolm wrote:
>> > gcc/
>> > * rtl.h (for_each_rtx_in_insn): New function.
>> > * rtlanal.c (for_each_rtx_in_insn): Likewise.
>> OK. Note that we're moving away from for_each_rtx... I haven't
>> looked, but there's a reasonable chance we may not need it after Richard
>> S.'s work is committed. Something to watch.
>
> CCing Richard S, as a "heads up".
>
> Richard, for context, the patch in this thread is this one:
> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00494.html
>
> which adds a "for_each_rtx_in_insn" function, to work on a top-level
> insn node, as part of a big cleanup I'm working on that distinguishes
> between insn nodes vs arbitrary rtx nodes:
> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00498.html
>
> I believe that your rtl-iter reworking:
> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00133.html
> hasn't landed in trunk yet (I don't see an rtl-iter.h there yet).
Right. I think they're held up on patch 40 (ironically the one that
conflicts with yours).
> More detailed notes follow:
>
> My reading of Richard's patch series is that it optimizes many specific
> for_each_rtx implementations, but keeps the existing generic one around.
Yes, but that's only temporary. The eventual aim is to get rid of
for_each_rtx altogether. The series I posted dealt with all calls
in gcc/ itself and the next series will get rid of all other calls
(in config/).
> Some notes on all of the places in which I've made use of the new
> "for_each_rtx_in_insn" in my patchkit:
>
> cfgcleanup.c:1728: for_each_rtx_in_insn (&BB_END (bb1), replace_label, &rr);
> cfgcleanup.c:1742: for_each_rtx_in_insn (&BB_END (bb1), replace_label, &rr);
> (both in outgoing_edges_match; I don't think Richard's patchkit
> touches this one).
>
>
> cfgcleanup.c:2002: for_each_rtx_in_insn (&insn, replace_label, &rr);
> (in try_crossjump_to_edge; likewise, I don't think Richard's kit
> touches this)
These are patch 38: https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00175.html
> rtlanal.c:3103: return for_each_rtx_in_insn (insn, for_each_inc_dec_find_mem, &data);
> ...in for_each_inc_dec, the use added in my:
> "[PATCH 176/236] cselib and incdec":
> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00711.html
>
> Looks like Richard's
> "[PATCH 40/50] rtlanal.c:for_each_inc_dec":
> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00177.html
> optimizes this.
>
> Merging these looks more involved: in my patch series I establish that
> the first param to for_each_inc_dec is an rtx_insn **, which may cause
> type issues in Richard's new code as is - but perhaps may allow some
> simplification? Given that the loc passed in is an insn, am I right in
> thinking we're only interested in MEM within the PATTERN of insn, right?
> Also, am I right in thinking that that particular callback can't modify
> the ptr that's passed in (I don't see writes back to *r)? If so, that
> suggests that we can lose an indirection from the first param, and
> simply work on PATTERN (insn).
Yeah, that's true, we could lose the indirection. I'll fix up patch 40
accordingly.
Thanks,
Richard