diff mbox

[007/236] New function: for_each_rtx_in_insn

Message ID 1407345815-14551-8-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm Aug. 6, 2014, 5:19 p.m. UTC 
gcc/
	* rtl.h (for_each_rtx_in_insn): New function.
	* rtlanal.c (for_each_rtx_in_insn): Likewise.
---
 gcc/rtl.h     |  1 +
 gcc/rtlanal.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

Comments

Jeff Law Aug. 12, 2014, 9:08 p.m. UTC | #1 
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.

Jeff
David Malcolm Aug. 14, 2014, 9:04 p.m. UTC | #2 
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
Richard Sandiford Aug. 14, 2014, 9:36 p.m. UTC | #3 
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
Jeff Law Aug. 15, 2014, 5:19 a.m. UTC | #4 
On 08/14/14 15:36, Richard Sandiford wrote:
>
> Right.  I think they're held up on patch 40 (ironically the one that
> conflicts with yours).
I think we could declare side effects in notes as invalid and add some 
ENABLE_CHECKING bits to enforce that.  With those in place, my concerns 
around #40 from your series would be eliminated.

Jeff
David Malcolm Aug. 18, 2014, 8:24 p.m. UTC | #5 
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.

Thanks.  Committed to trunk as r214119 [having bootstrapped&regrtested
on x86_64 linux (Fedora 20) as part of patches 2-8 (as per:
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01420.html )
and having verified build by itself]

Dave
diff mbox

Patch

diff --git a/gcc/rtl.h b/gcc/rtl.h
index 0858230..3e37ed0 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2356,6 +2356,7 @@  extern int computed_jump_p (const_rtx);
 
 typedef int (*rtx_function) (rtx *, void *);
 extern int for_each_rtx (rtx *, rtx_function, void *);
+extern int for_each_rtx_in_insn (rtx_insn **, rtx_function, void *);
 
 /* Callback for for_each_inc_dec, to process the autoinc operation OP
    within MEM that sets DEST to SRC + SRCOFF, or SRC if SRCOFF is
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index 82cfc1bf..5e2e908 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -3011,6 +3011,22 @@  for_each_rtx (rtx *x, rtx_function f, void *data)
   return for_each_rtx_1 (*x, i, f, data);
 }
 
+/* Like "for_each_rtx", but for calling on an rtx_insn **.  */
+
+int
+for_each_rtx_in_insn (rtx_insn **insn, rtx_function f, void *data)
+{
+  rtx insn_as_rtx = *insn;
+  int result;
+
+  result = for_each_rtx (&insn_as_rtx, f, data);
+
+  if (insn_as_rtx != *insn)
+    *insn = as_a_nullable <rtx_insn *> (insn_as_rtx);
+
+  return result;
+}
+
 
 
 /* Data structure that holds the internal state communicated between