diff mbox series

Handle 'NUM' in 'PUSH_INSERT_PASSES_WITHIN' (was: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

Message ID 87ed8htdwy.fsf@euler.schwinge.ddns.net
State New
Headers show
Series Handle 'NUM' in 'PUSH_INSERT_PASSES_WITHIN' (was: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes) | expand

Commit Message

Thomas Schwinge June 28, 2024, 1:06 p.m. UTC 
Hi!

As part of this:

On 2013-07-26T11:04:33-0400, David Malcolm <dmalcolm@redhat.com> wrote:
> This patch is the hand-written part of the conversion of passes from
> C structs to C++ classes.

> --- a/gcc/passes.c
> +++ b/gcc/passes.c

..., we did hard-code 'PUSH_INSERT_PASSES_WITHIN(PASS)' to always refer
to the first instance of 'PASS':

>  #define PUSH_INSERT_PASSES_WITHIN(PASS) \
>    { \
> -    struct opt_pass **p = &(PASS).pass.sub;
> +    struct opt_pass **p = &(PASS ## _1)->sub;

..., however we did change 'NEXT_PASS(PASS, NUM)' to actually use 'NUM':

> -#define NEXT_PASS(PASS, NUM)  (p = next_pass_1 (p, &((PASS).pass)))
> +#define NEXT_PASS(PASS, NUM) \
> +  do { \
> +    gcc_assert (NULL == PASS ## _ ## NUM); \
> +    if ((NUM) == 1)                              \
> +      PASS ## _1 = make_##PASS (ctxt_);          \
> +    else                                         \
> +      {                                          \
> +        gcc_assert (PASS ## _1);                 \
> +        PASS ## _ ## NUM = PASS ## _1->clone (); \
> +      }                                          \
> +    p = next_pass_1 (p, PASS ## _ ## NUM);  \
> +  } while (0)

This was never re-synchronized later on, and is problematic if you try to
do something like this; change:

    [...]
    NEXT_PASS (pass_postreload);
    PUSH_INSERT_PASSES_WITHIN (pass_postreload)
        NEXT_PASS (pass_postreload_cse);
        [...]
        NEXT_PASS (pass_cprop_hardreg);
        NEXT_PASS (pass_fast_rtl_dce);
        NEXT_PASS (pass_reorder_blocks);
        [...]
    POP_INSERT_PASSES ()
    [...]

... into:

    [...]
    NEXT_PASS (pass_postreload);
    PUSH_INSERT_PASSES_WITHIN (pass_postreload)
        NEXT_PASS (pass_postreload_cse);
        [...]
        NEXT_PASS (pass_cprop_hardreg);
    POP_INSERT_PASSES ()
    NEXT_PASS (pass_fast_rtl_dce);
    NEXT_PASS (pass_postreload);
    PUSH_INSERT_PASSES_WITHIN (pass_postreload)
        NEXT_PASS (pass_reorder_blocks);
        [...]
    POP_INSERT_PASSES ()
    [...]

That is, interrupt the pass pipeline within 'pass_postreload', in order
to unconditionally run 'pass_fast_rtl_dce' even if not running
'pass_postreload'.  What happens is that the second
'PUSH_INSERT_PASSES_WITHIN (pass_postreload)' overwrites the first
'PUSH_INSERT_PASSES_WITHIN (pass_postreload)' instead of applying to the
second (preceding) 'NEXT_PASS (pass_postreload);'.

(I ran into this in context of what I tried in
<https://inbox.sourceware.org/87ed8i2ekt.fsf@euler.schwinge.ddns.net>
"nvptx vs. [PATCH] Add a late-combine pass [PR106594]"; discuss that
specific use case over there, not here.)

OK to address this with the attached
"Handle 'NUM' in 'PUSH_INSERT_PASSES_WITHIN'"?

This depends on
<https://inbox.sourceware.org/87jzi9tgcw.fsf@euler.schwinge.ddns.net>
"Rewrite usage comment at the top of 'gcc/passes.def'" to avoid running
into the 'ERROR: Can't locate [...]' that I'm adding, while processing
the 'PUSH_INSERT_PASSES_WITHIN (PASS)' in the usage comment at the top of
'gcc/passes.def', where 'NEXT_PASS (PASS)' only appears later.  ;-)

I've verified this does the expected thing for the main 'gcc/passes.def',
and that 'PUSH_INSERT_PASSES_WITHIN' is not used/not applicable for
'PASSES_EXTRA' ('gcc/config/*/*-passes.def').


Grüße
 Thomas

Comments

David Malcolm Sept. 4, 2024, 3:18 p.m. UTC | #1 
On Fri, 2024-06-28 at 15:06 +0200, Thomas Schwinge wrote:
> Hi!
> 
> As part of this:
> 
> On 2013-07-26T11:04:33-0400, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > This patch is the hand-written part of the conversion of passes
> > from
> > C structs to C++ classes.
> 
> > --- a/gcc/passes.c
> > +++ b/gcc/passes.c
> 
> ..., we did hard-code 'PUSH_INSERT_PASSES_WITHIN(PASS)' to always
> refer
> to the first instance of 'PASS':
> 
> >  #define PUSH_INSERT_PASSES_WITHIN(PASS) \
> >    { \
> > -    struct opt_pass **p = &(PASS).pass.sub;
> > +    struct opt_pass **p = &(PASS ## _1)->sub;
> 
> ..., however we did change 'NEXT_PASS(PASS, NUM)' to actually use
> 'NUM':
> 
> > -#define NEXT_PASS(PASS, NUM)  (p = next_pass_1 (p,
> > &((PASS).pass)))
> > +#define NEXT_PASS(PASS, NUM) \
> > +  do { \
> > +    gcc_assert (NULL == PASS ## _ ## NUM); \
> > +    if ((NUM) == 1)                              \
> > +      PASS ## _1 = make_##PASS (ctxt_);          \
> > +    else                                         \
> > +      {                                          \
> > +        gcc_assert (PASS ## _1);                 \
> > +        PASS ## _ ## NUM = PASS ## _1->clone (); \
> > +      }                                          \
> > +    p = next_pass_1 (p, PASS ## _ ## NUM);  \
> > +  } while (0)
> 
> This was never re-synchronized later on, and is problematic if you
> try to
> do something like this; change:
> 
>     [...]
>     NEXT_PASS (pass_postreload);
>     PUSH_INSERT_PASSES_WITHIN (pass_postreload)
>         NEXT_PASS (pass_postreload_cse);
>         [...]
>         NEXT_PASS (pass_cprop_hardreg);
>         NEXT_PASS (pass_fast_rtl_dce);
>         NEXT_PASS (pass_reorder_blocks);
>         [...]
>     POP_INSERT_PASSES ()
>     [...]
> 
> ... into:
> 
>     [...]
>     NEXT_PASS (pass_postreload);
>     PUSH_INSERT_PASSES_WITHIN (pass_postreload)
>         NEXT_PASS (pass_postreload_cse);
>         [...]
>         NEXT_PASS (pass_cprop_hardreg);
>     POP_INSERT_PASSES ()
>     NEXT_PASS (pass_fast_rtl_dce);
>     NEXT_PASS (pass_postreload);
>     PUSH_INSERT_PASSES_WITHIN (pass_postreload)
>         NEXT_PASS (pass_reorder_blocks);
>         [...]
>     POP_INSERT_PASSES ()
>     [...]
> 
> That is, interrupt the pass pipeline within 'pass_postreload', in
> order
> to unconditionally run 'pass_fast_rtl_dce' even if not running
> 'pass_postreload'.  What happens is that the second
> 'PUSH_INSERT_PASSES_WITHIN (pass_postreload)' overwrites the first
> 'PUSH_INSERT_PASSES_WITHIN (pass_postreload)' instead of applying to
> the
> second (preceding) 'NEXT_PASS (pass_postreload);'.
> 
> (I ran into this in context of what I tried in
> <https://inbox.sourceware.org/87ed8i2ekt.fsf@euler.schwinge.ddns.net>
> "nvptx vs. [PATCH] Add a late-combine pass [PR106594]"; discuss that
> specific use case over there, not here.)
> 
> OK to address this with the attached
> "Handle 'NUM' in 'PUSH_INSERT_PASSES_WITHIN'"?
> 
> This depends on
> <https://inbox.sourceware.org/87jzi9tgcw.fsf@euler.schwinge.ddns.net>
> "Rewrite usage comment at the top of 'gcc/passes.def'" to avoid
> running
> into the 'ERROR: Can't locate [...]' that I'm adding, while
> processing
> the 'PUSH_INSERT_PASSES_WITHIN (PASS)' in the usage comment at the
> top of
> 'gcc/passes.def', where 'NEXT_PASS (PASS)' only appears later.  ;-)
> 
> I've verified this does the expected thing for the main
> 'gcc/passes.def',
> and that 'PUSH_INSERT_PASSES_WITHIN' is not used/not applicable for
> 'PASSES_EXTRA' ('gcc/config/*/*-passes.def').

Thanks; patch LGTM.

Dave
diff mbox series

Patch

From e368ccba93f5bbaee882076c80849adb55a68fa0 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tschwinge@baylibre.com>
Date: Fri, 28 Jun 2024 12:10:12 +0200
Subject: [PATCH] Handle 'NUM' in 'PUSH_INSERT_PASSES_WITHIN'

..., such that also for repeated 'NEXT_PASS', 'PUSH_INSERT_PASSES_WITHIN' for a
given 'PASS', the 'PUSH_INSERT_PASSES_WITHIN' applies to the preceeding
'NEXT_PASS', and not unconditionally applies to the first 'NEXT_PASS'.

	gcc/
	* gen-pass-instances.awk: Handle 'PUSH_INSERT_PASSES_WITHIN'.
	* pass_manager.h (PUSH_INSERT_PASSES_WITHIN): Adjust.
	* passes.cc (PUSH_INSERT_PASSES_WITHIN): Likewise.
---
 gcc/gen-pass-instances.awk | 28 +++++++++++++++++++++++++---
 gcc/pass_manager.h         |  2 +-
 gcc/passes.cc              |  6 +++---
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/gcc/gen-pass-instances.awk b/gcc/gen-pass-instances.awk
index 449889663f7..871ac0cdb52 100644
--- a/gcc/gen-pass-instances.awk
+++ b/gcc/gen-pass-instances.awk
@@ -16,7 +16,7 @@ 
 
 # This Awk script takes passes.def and writes pass-instances.def,
 # counting the instances of each kind of pass, adding an instance number
-# to everywhere that NEXT_PASS is used.
+# to everywhere that NEXT_PASS or PUSH_INSERT_PASSES_WITHIN are used.
 # Also handle INSERT_PASS_AFTER, INSERT_PASS_BEFORE and REPLACE_PASS
 # directives.
 #
@@ -222,9 +222,31 @@  END {
 	  if (with_arg)
 	    printf ",%s", with_arg;
 	  printf ")%s\n", postfix;
+
+	  continue;
 	}
-      else
-	print lines[i];
+
+      ret = parse_line(lines[i], "PUSH_INSERT_PASSES_WITHIN");
+      if (ret)
+	{
+	  pass_name = args[1];
+
+	  pass_num = pass_final_counts[pass_name];
+	  if (!pass_num)
+	    {
+	      print "ERROR: Can't locate instance of the pass mentioned in " pass_name;
+	      exit 1;
+	    }
+
+	  printf "%s", prefix;
+	  printf "PUSH_INSERT_PASSES_WITHIN";
+	  printf " (%s, %s", pass_name, pass_num;
+	  printf ")%s\n", postfix;
+
+	  continue;
+	}
+
+      print lines[i];
     }
 }
 
diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
index be324d5dff7..edd775e9a9c 100644
--- a/gcc/pass_manager.h
+++ b/gcc/pass_manager.h
@@ -126,7 +126,7 @@  private:
         opt_pass *pass_copy_prop_8;  */
 
 #define INSERT_PASSES_AFTER(PASS)
-#define PUSH_INSERT_PASSES_WITHIN(PASS)
+#define PUSH_INSERT_PASSES_WITHIN(PASS, NUM)
 #define POP_INSERT_PASSES()
 #define NEXT_PASS(PASS, NUM) opt_pass *PASS ## _ ## NUM
 #define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) NEXT_PASS (PASS, NUM)
diff --git a/gcc/passes.cc b/gcc/passes.cc
index b01a79ef96c..023631046a6 100644
--- a/gcc/passes.cc
+++ b/gcc/passes.cc
@@ -1574,7 +1574,7 @@  pass_manager::pass_manager (context *ctxt)
 
   /* Zero-initialize pass members.  */
 #define INSERT_PASSES_AFTER(PASS)
-#define PUSH_INSERT_PASSES_WITHIN(PASS)
+#define PUSH_INSERT_PASSES_WITHIN(PASS, NUM)
 #define POP_INSERT_PASSES()
 #define NEXT_PASS(PASS, NUM) PASS ## _ ## NUM = NULL
 #define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) NEXT_PASS (PASS, NUM)
@@ -1604,9 +1604,9 @@  pass_manager::pass_manager (context *ctxt)
     *p = NULL;					\
   }
 
-#define PUSH_INSERT_PASSES_WITHIN(PASS) \
+#define PUSH_INSERT_PASSES_WITHIN(PASS, NUM) \
   { \
-    opt_pass **p = &(PASS ## _1)->sub;
+    opt_pass **p = &(PASS ## _ ## NUM)->sub;
 
 #define POP_INSERT_PASSES() \
   }
-- 
2.34.1