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 <dmalc...@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....@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....@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
