Skip to content

Fix nested always-inline call chains - #976

Open
fbernier wants to merge 1 commit into
rust-lang:masterfrom
fbernier:fix/nested-always-inline
Open

fbernier wants to merge 1 commit into
rust-lang:masterfrom
fbernier:fix/nested-always-inline

Conversation

@fbernier

Copy link
Copy Markdown
Contributor

Preserve #[inline(always)] for nested, non-recursive calls by detecting cycles instead of treating any always-inline callee as recursion. The traversal is iterative and cached.

Add an -O0 assembly regression. Existing tests cover self-recursion and mutual recursion.

@rustbot

This comment has been minimized.

Detect cycles instead of treating every nested always-inline call as recursive. Cache results and keep the traversal on the heap.

Add an -O0 assembly regression for acyclic chains. Retain the existing always_inline runtime test for self-recursion and mutual recursion.
@fbernier
fbernier force-pushed the fix/nested-always-inline branch from b3a2235 to 935c4f4 Compare September 15, 2026 02:19
@rustbot

rustbot commented Sep 15, 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.

Comment thread src/context.rs
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
) {
// 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
}
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
/// 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

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