Skip to content

Fix nested always-inline call chains - #976

Closed
fbernier wants to merge 3 commits into
rust-lang:masterfrom
fbernier:fix/nested-always-inline
Closed

fbernier wants to merge 3 commits into
rust-lang:masterfrom
fbernier:fix/nested-always-inline

Conversation

@fbernier

@fbernier fbernier commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

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 calls
in 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_inline is 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 forwarding
    wrappers.
  • function body not available — foreign declarations and functions defined in another codegen unit must not receive always_inline.
  • Variable-argument inlining errors — C-variadic functions are demoted to an ordinary inline hint.
  • target specific option mismatch — x86 interrupt handlers use general-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

  • Use the compiler’s monomorphized item collector, supplemented by MIR and allocation scanning to distinguish direct calls from taken addresses.
  • Resolve foreign declarations to definitions in the current codegen unit, including EII wrapper symbols.
  • Record calls and addresses separately, normalizing references before classification. Vtable construction is handled conservatively.
  • Use rustc’s existing SCC implementation and cache results per codegen unit. Separate forced-inline cycles from address cycles through ordinary helpers; a plain helper can
    break a cycle made entirely of direct calls.
  • Treat imported monomorphizations as leaves, avoiding checks of upstream bodies against downstream target features.

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

  • Assembly tests: 14 passed, covering recovered inlining and the crash cases.
  • Language tests: 15 compile tests, plus 43 debug and 43 release run tests passed.
  • --std-tests and --test-libcore passed.
  • Formatting, check-todo, clippy, and cargo check --no-default-features passed.
  • Additional reproductions were checked at -O0 through -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.

@rustbot

This comment has been minimized.

@fbernier
fbernier force-pushed the fix/nested-always-inline branch from b3a2235 to 935c4f4 Compare September 15, 2026 02:19
@rustbot

This comment has been minimized.

Comment thread src/context.rs Outdated
pub functions: RefCell<FxHashMap<String, Function<'gcc>>>,
pub intrinsics: RefCell<FxHashMap<String, Function<'gcc>>>,
#[cfg(feature = "master")]
pub inline_recursion: RefCell<FxHashMap<DefId, bool>>,

@antoyo antoyo Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment to explain why we need this field and what it is used for and what does the bool represent.

View changes since the review

Comment thread src/attributes.rs Outdated
) {
// 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)];

@antoyo antoyo Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment to explain what the bool represents in this Vec.

View changes since the review

Comment thread src/attributes.rs Outdated
}
active.insert(def);
pending.push((def, true));
for block in cx.tcx.optimized_mir(def).basic_blocks.iter().rev() {

@antoyo antoyo Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View changes since the review

Comment thread src/attributes.rs Outdated
/// 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.

@antoyo antoyo Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain in the doc comment what is the return value.

View changes since the review

Comment thread src/attributes.rs
Comment on lines -38 to -39
// 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.

@antoyo antoyo Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this comment as it is useful.

View changes since the review

@fbernier
fbernier force-pushed the fix/nested-always-inline branch from 935c4f4 to 7486c5a Compare September 23, 2026 04:20
@rustbot

rustbot commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

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.

@fbernier
fbernier force-pushed the fix/nested-always-inline branch 3 times, most recently from 84fbc78 to a63e32c Compare September 23, 2026 04:47
fbernier added a commit to fbernier/rustc_codegen_gcc that referenced this pull request Sep 23, 2026
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.
@rustbot

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.
@fbernier
fbernier force-pushed the fix/nested-always-inline branch from 0662076 to 0e65847 Compare September 23, 2026 05:07
@rustbot

rustbot commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

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.
@fbernier
fbernier force-pushed the fix/nested-always-inline branch from 70ed5bf to 4db4e22 Compare September 23, 2026 13:57
@fbernier

Copy link
Copy Markdown
Contributor Author

this slop would be better handled by gcc itself. Closing.

@fbernier fbernier closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants