Re: [PATCH] Move reload_completed and other rtl.h globals to crtl structure.

[Richard Sandiford <richard.sandiford@arm.com>]

I know it'll seem like make-work, but could you put the combine flag
in a separate follow-on patch?  Reorganising the existing flags
(very welcome!) and adding new ones seem like different things.

TBH I'm a bit suspicious of the combine flag.  What fundamental
property holds true after combine that doesn't hold before it,
or vice versa?  There are other passes that do combine-like things,
such as fwprop and postreload.

The reason for asking is that the RA flags -- before RA, during RA,
after RA -- describe a clear lowering process.  Before RA pseudos
are freely used (and preferred where valid), and insn validity depends
only on C conditions and predicates.  After RA pseudos aren't allowed
and insn validity also depends on constraints.  During RA is a
half-way house.

cse_not_expected always felt like a bit of a hack, but I think the
principle was that, when cse_not_expected is true, you shouldn't
force complex operations to be split into simpler ones (with the
intention of promoting reuse of the simpler operations).  So the
purpose of the flag can be described in terms of what the .md file
should do, rather than being tied to the behaviour of a specific pass.

Sorry for being awkward.  It's just that...

"Roger Sayle" <roger@nextmovesoftware.com> writes:
> On 11 July 2022 08:20, Richard Biener wrote:
>> On Sun, 10 Jul 2022, Roger Sayle wrote:
>> 
>> > This patch builds upon Richard Biener's suggestion of avoiding global
>> > variables to track state/identify which passes have already been run.
>> > In the early middle-end, the tree-ssa passes use the curr_properties
>> > field in cfun to track this.  This patch uses a new rtl_pass_progress
>> > int field in crtl to do something similar.
>> 
>> Why not simply add PROP_rtl_... and use the existing curr_properties for
> this?
>> RTL passes are also passes and this has the advantage you can add things
> like
>> reload_completed to the passes properties_set field hand have the flag
> setting
>> handled by the pass manager as it was intended?
>> 
>
> Great question, and I did initially consider simply adding more RTL fields
> to
> curr_properties.  My hesitation was from the comments/documentation that
> the curr_properties field is used by the pass manager as a way to track
> (and verify) the properties/invariants that are required, provided and
> destroyed
> by each pass.  This semantically makes sense for properties such as accurate
> data flow, ssa form, cfg_layout, nonzero_bits etc, where hypothetically the
> pass manager can dynamically schedule a pass/analysis to ensure the next
> pass
> has the pre-requisite information it needs.
>
> This seems semantically slightly different from tracking time/progress,
> where
> we really want something more like DEBUG_COUNTER that simply provides
> the "tick-tock" of a pass clock.  Alas, the "pass number", as used in the
> suffix
> of dump-files (where 302 currently means reload) isn't particularly useful
> as
> these change/evolve continually.
>
> Perhaps the most obvious indication of this (subtle) difference is the
> curr_properties field (PROP_rtl_split_insns) which tracks whether
> instructions
> have been split, where at a finer granularity rtl_pass_progress may wish to
> distinguish split1 (after combine before reload), split2 (after reload
> before
> peephole2) and split3 (after peephole2).  It's conceptually not a simple
> property, whether all insns have been split or not, as in practice this is
> more subtle with backends choosing which instructions get split at which
> "times".

...this made it seem like adding the combine flag was a slippery slope
towards adding flags for potentially any pass.  I think we should avoid
exposing this (3-split) level of granularity if we can help it, since it
would make it harder to rejig the pipeline in future.

Where possible, it would be good to give target-independent code more
information about what the target needs, rather than give the target
more information about the inner workings of target-independent code.

That's also a reason why I agree with Richard that it would be better
to use property flags.  That might help to enforce the principle that
the flags should describe properties of the current IL, rather than
a list of what has and hasn't happened so far.

Thanks,
Richard

> There's also the concern that we've a large number of passes (currently
> 62 RTL passes), and only a finite number of bits (in curr_properties), so
> having two integers reduces the risk of running out of bits and needing
> to use a "wider" data structure.
>
> To be honest, I just didn't want to hijack curr_properties to abuse it for a
>
> use that didn't quite match the original intention, without checking with
> you and the other maintainers first.  If the above reasoning isn't
> convincing,
> I can try spinning an alternate patch using curr_properties (but I'd expect
> even more churn as backend source files would now need to #include
> tree-passes.h and function.h to get reload_completed).  And of course,
> a volunteer is welcome to contribute that re-refactoring after this one.
>
> I've no strong feelings either way.  It was an almost arbitrary engineering
> decision (but hopefully the above explains the balance of my reasoning).
>
> Roger
> --