diff mbox series

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

Message ID 87y147yapx.fsf@euler.schwinge.ddns.net
State New
Headers show
Series [PING] 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 Sept. 4, 2024, 10:28 a.m. UTC 
Hi!

Ping.

On 2024-06-28T15:06:21+0200, I wrote:
> 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.  ;-)

(Already pushed.)

> 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
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