inliner: Break inlining cycles #78580

tmiasko · 2020-10-30T18:18:33Z

Keep track of all instances inlined so far. When examining a new call
sites from an inlined body, skip those where callee had been inlined
already to avoid potential inlining cycles.

Fixes #78573.

lcnr · 2020-10-31T19:14:16Z

LTGM, r? @oli-obk for final approval

antonholmberg

Hi! I am really new to the rust community so I am not sure if it's okay to review things that you aren't assigned to so feel free to ignore my comment if you want to.

compiler/rustc_mir/src/transform/inline.rs

oli-obk · 2020-11-01T14:26:40Z

I am not sure if it's okay to review things that you aren't assigned to so feel free to ignore my comment if you want to.

It's totally ok. I found it especially good that your review was asking whether a change would be good, instead of saying "do XYZ".

oli-obk · 2020-11-01T14:31:42Z

Hmm... will #68828 automatically prevent these situations? Or could we somehow extend that to also catch this case?

tmiasko · 2020-11-01T15:18:36Z

This addresses inlining cycles that become apparent only after the substitution or between functions from a different crate. For #68828 to address this issue, it would have to never inline a body where Instance::resolve had failed (including both local and non-local crates).

oli-obk · 2020-11-01T16:36:48Z

We could also list generic params which are used in function calls together with concrete function calls. This way the inliner can decide directly whether to inline, and not do it "after the fact". The main reason I'm asking this is src/test/mir-opt/inline/inline_cycle.one.Inline.diff which has

-         _1 = <C as Call>::call() -> bb1; // scope 0 at $DIR/inline-cycle.rs:14:5: 14:24
+         _1 = <C as Call>::call() -> bb1; // scope 3 at $DIR/inline-cycle.rs:36:9: 36:28

which, imo should never happen.

tmiasko · 2020-11-01T17:36:36Z

What exactly should never happen?

antonholmberg · 2020-11-01T17:39:36Z

it would have to never inline a body where Instance::resolve had failed (including both local and non-local crates).

Isn't this already the case since get_valid_function_call runs?

let callee =
    Instance::resolve(self.tcx, self.param_env, callee_def_id, normalized_substs)
        .ok()
        .flatten()?;

oli-obk · 2020-11-02T12:41:40Z

What exactly should never happen?

In this case we are inlining a function call, only to end up with the exact same function call again. All this does is to change the spans and debug info on the call, which will make the debugging experience weird

tmiasko · 2020-11-02T12:55:29Z

In this case we are inlining a function call, only to end up with the exact same function call again. All this does is to change the spans and debug info on the call, which will make the debugging experience weird

Yes, it this case decision to inline is a bad one. We could prevent such direct self-recursion by looking ahead in the callee body (the body is already optimized, so it is self-recursion after applying the substs). Generally speaking the aspiration of this PR is to stop the inlining cycles, rather than modify the inlining heuristic, but I can modify the heuristic as well.

oli-obk · 2020-11-02T13:06:33Z

but I can modify the heuristic as well.

Instead of doing both, could just modifying the heuristic to detect inlining cycles also fix the infinite loop?

bors · 2020-11-03T09:50:30Z

☔ The latest upstream changes (presumably #78697) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

tmiasko · 2020-11-06T20:26:56Z

could just modifying the heuristic to detect inlining cycles also fix the infinite loop?

I can modify the approach to look for self-recursive calls and calls to instances that are already part of inlining history one step ahead in should_inline.

bors · 2020-11-09T03:25:02Z

☔ The latest upstream changes (presumably #78889) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

oli-obk

I like this approach much better. Thanks!

Just a nit, then this lgtm

compiler/rustc_mir/src/transform/inline.rs

The inliner does not support inlining of divering calls. Reject them early on and turn `inline_call` into an infallible operation.

When examining candidates for inlining, reject those that are determined to be recursive either because of self-recursive calls or calls to any instances already inlined.

oli-obk · 2020-11-09T15:46:33Z

@bors r+

bors · 2020-11-09T15:46:34Z

📌 Commit dc4d74d has been approved by oli-obk

Rollup of 12 pull requests Successful merges: - rust-lang#74754 (Add `#[cfg(panic = '...')]`) - rust-lang#76468 (Improve lifetime name annotations for closures & async functions) - rust-lang#77016 (Test clippy on PR CI on changes) - rust-lang#78480 (BTreeMap: fix pointer provenance rules) - rust-lang#78502 (Update Chalk to 0.36.0) - rust-lang#78513 (Infer the default host target from the host toolchain if possible) - rust-lang#78566 (Enable LLVM Polly via llvm-args.) - rust-lang#78580 (inliner: Break inlining cycles) - rust-lang#78710 (rustc_ast: Do not panic by default when visiting macro calls) - rust-lang#78746 (Demote i686-unknown-freebsd to tier 2 compiler target) - rust-lang#78830 (fix `super_visit_with` for `Terminator`) - rust-lang#78844 (Monomorphize a type argument of size-of operation during codegen) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned lcnr Oct 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2020

camelid added the A-mir-opt-inlining Area: MIR inlining label Oct 30, 2020

lcnr approved these changes Oct 31, 2020

View reviewed changes

rust-highfive assigned oli-obk and unassigned lcnr Oct 31, 2020

antonholmberg reviewed Nov 1, 2020

View reviewed changes

compiler/rustc_mir/src/transform/inline.rs Outdated Show resolved Hide resolved

tmiasko force-pushed the inline-loop branch from ebe339e to 042cf6a Compare November 1, 2020 21:14

tmiasko changed the title ~~inliner: Skip calls that could be recursive~~ inliner: Break inlining cycles Nov 1, 2020

tmiasko force-pushed the inline-loop branch 3 times, most recently from 063dff5 to 018fd86 Compare November 6, 2020 23:29

oli-obk reviewed Nov 9, 2020

View reviewed changes

compiler/rustc_mir/src/transform/inline.rs Outdated Show resolved Hide resolved

tmiasko added 2 commits November 9, 2020 11:41

inliner: Make inline_call infallible

b7f16c5

The inliner does not support inlining of divering calls. Reject them early on and turn `inline_call` into an infallible operation.

inliner: Break inlining cycles

dc4d74d

When examining candidates for inlining, reject those that are determined to be recursive either because of self-recursive calls or calls to any instances already inlined.

tmiasko force-pushed the inline-loop branch from 018fd86 to dc4d74d Compare November 9, 2020 11:13

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2020

tmiasko mentioned this pull request Nov 9, 2020

Support inlining diverging function calls #78899

Closed

Dylan-DPC-zz mentioned this pull request Nov 9, 2020

Rollup of 12 pull requests #78904

Merged

Dylan-DPC-zz mentioned this pull request Nov 10, 2020

Prevent query cycles in the MIR inliner #68828

Merged

bors merged commit ee1fedf into rust-lang:master Nov 10, 2020

rustbot added this to the 1.49.0 milestone Nov 10, 2020

tmiasko deleted the inline-loop branch November 10, 2020 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inliner: Break inlining cycles #78580

inliner: Break inlining cycles #78580

tmiasko commented Oct 30, 2020 •

edited

Loading

lcnr commented Oct 31, 2020

antonholmberg left a comment

oli-obk commented Nov 1, 2020

oli-obk commented Nov 1, 2020

tmiasko commented Nov 1, 2020 •

edited

Loading

oli-obk commented Nov 1, 2020

tmiasko commented Nov 1, 2020

antonholmberg commented Nov 1, 2020 •

edited

Loading

oli-obk commented Nov 2, 2020

tmiasko commented Nov 2, 2020

oli-obk commented Nov 2, 2020

bors commented Nov 3, 2020

tmiasko commented Nov 6, 2020

bors commented Nov 9, 2020

oli-obk left a comment

oli-obk commented Nov 9, 2020

bors commented Nov 9, 2020

inliner: Break inlining cycles #78580

inliner: Break inlining cycles #78580

Conversation

tmiasko commented Oct 30, 2020 • edited Loading

lcnr commented Oct 31, 2020

antonholmberg left a comment

Choose a reason for hiding this comment

oli-obk commented Nov 1, 2020

oli-obk commented Nov 1, 2020

tmiasko commented Nov 1, 2020 • edited Loading

oli-obk commented Nov 1, 2020

tmiasko commented Nov 1, 2020

antonholmberg commented Nov 1, 2020 • edited Loading

oli-obk commented Nov 2, 2020

tmiasko commented Nov 2, 2020

oli-obk commented Nov 2, 2020

bors commented Nov 3, 2020

tmiasko commented Nov 6, 2020

bors commented Nov 9, 2020

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk commented Nov 9, 2020

bors commented Nov 9, 2020

tmiasko commented Oct 30, 2020 •

edited

Loading

tmiasko commented Nov 1, 2020 •

edited

Loading

antonholmberg commented Nov 1, 2020 •

edited

Loading