Conversation
This comment has been minimized.
This comment has been minimized.
b3a2235 to
935c4f4
Compare
This comment has been minimized.
This comment has been minimized.
| pub functions: RefCell<FxHashMap<String, Function<'gcc>>>, | ||
| pub intrinsics: RefCell<FxHashMap<String, Function<'gcc>>>, | ||
| #[cfg(feature = "master")] | ||
| pub inline_recursion: RefCell<FxHashMap<DefId, bool>>, |
There was a problem hiding this comment.
Please add a comment to explain why we need this field and what it is used for and what does the bool represent.
| ) { | ||
| // Keep the DFS on the heap: valid forced-inline chains can be arbitrarily | ||
| // deep, independently of the compiler thread's remaining call stack. | ||
| let mut pending: Vec<(DefId, bool)> = vec![(instance.def_id(), false)]; |
There was a problem hiding this comment.
Add a comment to explain what the bool represents in this Vec.
| } | ||
| active.insert(def); | ||
| pending.push((def, true)); | ||
| for block in cx.tcx.optimized_mir(def).basic_blocks.iter().rev() { |
There was a problem hiding this comment.
Is this loop in reverse order because calls are terminators, so at the end of basic blocks?
Add a comment to explain why this is in reverse order.
| /// Checks if the function `instance` is recursively inline. | ||
| /// Returns `false` if a functions is guaranteed to be non-recursive, and `true` if it *might* be recursive. | ||
| /// Check forced-inline call chains for cycles. Merely calling another | ||
| /// always-inline function is not recursion. |
There was a problem hiding this comment.
Explain in the doc comment what is the return value.
| // I assume that the recursive-inline issue applies only to functions, and not to drops. | ||
| // In principle, a recursive, `#[inline(always)]` drop could(?) exist, but I don't think it does. |
There was a problem hiding this comment.
Please keep this comment as it is useful.
935c4f4 to
7486c5a
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
84fbc78 to
a63e32c
Compare
The runner image ships a package index naming glib2.0 2.80.0-6ubuntu3.8, which the Ubuntu mirror no longer serves, so all four --projects jobs of rust-lang#976 failed in this step with 404s before building anything.
This comment has been minimized.
This comment has been minimized.
Only drop #[inline(always)] to a plain inline hint when GCC can't honor it. Before, any always-inline function that called another one was demoted, so at -O0 acyclic wrapper chains (intrinsic wrappers in particular) became real calls. always_inline isn't a hint for GCC: it fails the build when it can't inline. So a function keeps it unless: - it is only declared in this codegen unit; - it uses va_arg; - an x86-interrupt handler here could inline it. Handlers are built with general-regs-only, and GCC won't inline normally-built code into them; - a foreign declaration can call it: an extern item importing its exported symbol, or the EII declaration it implements. GCC sees those as direct calls, but the Rust call graph doesn't; - GCC could end up inlining it into itself. All but the last also apply to #[rustc_force_inline]. The last check runs on a call graph over monomorphized instances, taken from the collector's items_of_instance, so trait methods and closures resolve to their real bodies. Besides calls, an edge also goes to every function whose address is taken: fn pointers, vtable methods, constants and static initializers. GCC turns calls through a known address into direct calls, at -O0 for an immediate &f and from -O1 through read-only tables and vtables. At -O0 only always-inline functions are nodes; when optimizing, anything not marked noinline is, since GCC may inline plain helpers too. Imported monomorphizations are leaves; querying them would also check them against the wrong target features. Cycles are found with rustc's graph::scc::Sccs over the unclassified part of the graph, and every function visited is cached per codegen unit. Only functions on a cycle are demoted, so callers that merely reach one keep always_inline. When optimizing, recursing through a plain helper also demotes. A regex release build takes the same time as before (41.3-41.6 s). requested_inline is shared by from_fn_attrs and the graph, so both see the same effective attribute (naked, requires_inline, weak linkage). from_fn_attrs now takes an FnBody saying whether the function gets a body. Tests: - tests/run/always_inline.rs: cycles through a trait method, an FnMut closure and a local fn pointer. - tests/asm: an acyclic chain at -O0, a caller of a recursive function keeping always_inline, cycles through a static fn table and a vtable at -O1, and an upstream avx monomorphization reached through a static table. The asm tester drops the build system's --out-dir so compiletest's aux-build works. - tests/compile: a variadic function, a foreign declaration, a link_name alias, an EII implementation calling its declaration, and x86-interrupt handlers. The variadic and interrupt tests are skipped on m68k. mini_core gets the unsafe_unpin lang item, which current rustc needs to optimize generic &mut F at -O3.
The runner image ships a package index naming glib2.0 2.80.0-6ubuntu3.8, which the Ubuntu mirror no longer serves, so all four --projects jobs of rust-lang#976 failed in this step with 404s before building anything.
0662076 to
0e65847
Compare
|
The previous commit demoted some functions without looking for a cycle. This one adds the missing edges instead, so these functions keep always_inline unless they are really on a cycle: - EII implementations, and exported functions that some extern declaration imports. The collector skips foreign declarations, so we now scan each body for calls, fn-pointer casts and constants that refer to one, and map the declaration's symbol to the definition in this codegen unit (including the EII wrapper symbol). A declaration defined in another unit is left alone: GCC can't inline across units. - Recursion through a plain helper when optimizing. The same scan tells direct calls apart from taken addresses: reified fn pointers, closures turned into fn pointers, and fn pointers in constants. The collector lists each function once, so one that is both called and has its address taken counts as an address. A body that coerces something to dyn Trait counts every function it uses as an address, since we don't work out what the new vtable holds. GCC is fine with a cycle through a plain function made only of direct calls; it only rejects cycles made purely of always_inline functions, and addresses that resolve back into a function they were inlined into. So there are now two checks: SCCs over always-inline functions only, and, when optimizing, an address edge whose ends share an SCC in the full graph. #[rustc_force_inline] now goes through the same cycle check: the MIR inliner only rejects call cycles, not ones through addresses or foreign declarations. A regex release build takes the same time (41.9-42.4 s on both). Tests: - tests/asm/always_inline_foreign_decls.rs: an EII implementation and an imported exported function keep always_inline at -O0. - tests/asm/always_inline_helper_recursion.rs: recursing through a plain helper at -O2 keeps it. - tests/asm/always_inline_call_and_address.rs: a helper that both calls a forced function and returns its address. It crashes GCC at -O2 if the address check is missing or the call hides the address. The first two fail on the previous commit.
70ed5bf to
4db4e22
Compare
|
this slop would be better handled by gcc itself. Closing. |
Summary
Preserve
#[inline(always)]on acyclic call chains without triggering GCC’s hard inlining errors.The previous heuristic demoted a function whenever it called another always-inline function. At
-O0, where an ordinary inline hint is insufficient, this left real callsin otherwise fully inlinable wrapper chains, including intrinsic wrappers.
This replaces that heuristic with analysis of monomorphized functions and their uses. It also preserves forced inlining for acyclic exported functions and EII
implementations, and for direct-call recursion through an ordinary helper.
GCC failures covered
always_inlineis not a hint in GCC: when GCC cannot inline, compilation fails. The implementation and regression tests cover:recursive inlining— cycles through trait dispatch, closures, function pointers, constants, static tables, vtables, foreign symbol aliases, and EII forwardingwrappers.
function body not available— foreign declarations and functions defined in another codegen unit must not receivealways_inline.target specific option mismatch— x86 interrupt handlers usegeneral-regs-only, so normally compiled functions they could inline must be demoted.Some of these failures were exposed while relaxing the old heuristic. The tests ensure that recovering inlining does not introduce codegen crashes.
One subtle regression involved a helper that both calls a function directly and returns its address. Recording only the direct call misses a cycle that GCC exposes
during optimization, causing a crash at
-O2/-O3. Address uses now take precedence, and a Rust regression test covers this case.Implementation
break a cycle made entirely of direct calls.
The demonstrated codegen improvement is removal of calls from acyclic wrapper chains at
-O0; this is not a claim of a general release-build speedup.Validation
--std-testsand--test-libcorepassed.check-todo, clippy, andcargo check --no-default-featurespassed.-O0through-O3.The C-variadic regression test is skipped on m68k, where C-variadic definitions remain unstable. The accompanying CI commit refreshes the apt index before installing
librsvg dependencies to avoid stale-index 404s.
Known limitation
The analysis is per codegen unit. Cross-unit address-taking cycles exposed only by fat LTO remain outside its coverage. That path could not be validated with the downloaded
libgccjit, whose LTO support is currently unavailable.