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: David Malcolm <dmalcolm at redhat dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Richard Sandiford <rdsandiford at googlemail dot com>
- Date: Thu, 14 Aug 2014 17:04:55 -0400
- 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>
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).
Summary of notes below: I think our patch series are mostly compatible,
but I think there's a conflict between them in rtlanal.c to do with
for_each_inc_dec, at patch 176 in my kit vs patch 40 of your kit. I
think it's resolvable, though.
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.
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)
ira.c:3350: for_each_rtx_in_insn (&insn, set_paradoxical_subreg,
...in update_equiv_regs, the use added in my
"[PATCH 191/236] Remove DF_REF_INSN scaffolding":
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00554.html
It looks like Richard's
"[PATCH 25/50] ira.c:set_paradoxical_subreg":
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00161.html
optimizes the current code, and my reading of his patch is that it's
compatible with my patchkit; the for_each_rtx_in_insn can be eliminated.
Hence, depending on whose patch goes in first, the other patch will need
some fixup, but I think it will be trivial to fixup.
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).
config/i386/i386.c:46886: for_each_rtx_in_insn (&insn, (rtx_function) ix86_loop_memcount,
(in ix86_loop_unroll_adjust)
config/s390/s390.c:11820: for_each_rtx_in_insn (&insn, (rtx_function) check_dpu, &mem_count);
(in s390_loop_unroll_adjust)
(as far as I can see Richard's patches don't touch the config subdirs).
Hope this is sane
Dave