From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 1949C3858412 for ; Mon, 11 Jul 2022 09:42:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1949C3858412 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id EB494227FF; Mon, 11 Jul 2022 09:42:09 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id CDF782C141; Mon, 11 Jul 2022 09:42:09 +0000 (UTC) Date: Mon, 11 Jul 2022 09:42:09 +0000 (UTC) From: Richard Biener To: Roger Sayle cc: gcc-patches@gcc.gnu.org Subject: RE: [PATCH] Move reload_completed and other rtl.h globals to crtl structure. In-Reply-To: <020401d89500$e3f6ca90$abe45fb0$@nextmovesoftware.com> Message-ID: References: <00e601d89489$a5133740$ef39a5c0$@nextmovesoftware.com> <020401d89500$e3f6ca90$abe45fb0$@nextmovesoftware.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jul 2022 09:42:12 -0000 On Mon, 11 Jul 2022, Roger Sayle wrote: > 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. Yeah, that might have been some of the design inspiring bits but actually the pass manager does nothing of this sorts instead it only verifies the statically scheduled producers/consumers match expectations ... We also track in these bits how the IL evolves from high to low GIMPLE, to CFG/SSA, to after the point where complex or vector ops are lowered, etc. To me whether we've assigned hard regs (after-reload) is quite similar, the after_combine would be similar to PROP_gimple_opt_math. You've already found PROP_rtl_split_insns. > 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". I'm not sure it's good to allow such fine-grained control since it makes the pass pipeline x N-target.md a quite fragile setup. You could argue you want such flag on the actual RTL insn (was-split-from-insn-X) even ... > 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. As said whether an individual pass has run or not is not what this bitmask is for nor is it what we should do. We should provide a more abstract thing to test for and I doubt we can make one for each of the 62 RTL passes ;) > 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). I definitely lean towards using curr_properties and new PROP_rtl_..., but if anybody else has opposite views I won't insist. + /* Track progress of RTL passes, reload_completed etc. */ + int rtl_pass_progress; btw, this suggests that we don't need a bitmask but instead can define a strong order, making the tests with a relational compare? The reason we do not have this with PROP_ is twofold, one, we're mixing with flags that come and go away, second, some of the "order" stuff might depend on the optimization level since we have separate pipelines for -O0 and -O1 (and -Og) and whether for example complex or vectors are lowered might happen in different order (I didn't actually check whether that's the case right now). It might suggest that splitting up curr_properties is a good idea (maybe then rename rtl_pass_progress to pass_progress so we can reuse it there). Richard. > Roger > -- > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstra