Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't generate code for unused #[inline] function #36280

Closed
retep998 opened this issue Sep 5, 2016 · 12 comments
Closed

Don't generate code for unused #[inline] function #36280

retep998 opened this issue Sep 5, 2016 · 12 comments
Labels
A-codegen Area: Code generation I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@retep998
Copy link
Member

retep998 commented Sep 5, 2016

If a function or method is marked #[inline] and it isn't actually called anywhere in that crate, then no code should be generated for it. This would save a lot of time on translation and LLVM passes for certain crates, such as winapi.

@eddyb told me to open this issue. Blame him.

@eddyb eddyb added A-codegen Area: Code generation I-nominated I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2016
@eddyb
Copy link
Member

eddyb commented Sep 5, 2016

cc @rust-lang/compiler - especially @michaelwoerister, I believe the collector should be able to reliably detect whether an #[inline] function is actually used, the only change needed to get this behavior (if we want it) would be to not use #[inline] functions as roots in the collector.

@michaelwoerister
Copy link
Member

Afaict, the only thing that needs to be adapted should be TransItem::is_instantiated_only_on_demand(). There's also TransItem::requests_inline() which makes it easy to check for the #[inline] attribute.

@retep998
Copy link
Member Author

retep998 commented Sep 5, 2016

I did a simple patch which changed that function and it didn't really affect compile times. In fact the LLVM IR is just as big, leading me to conclude that there is more going on to decide whether to instantiate something than that function.

I'll let someone else take it from here because this is beyond my knowledge of rustc: retep998@e8d9b86

@michaelwoerister
Copy link
Member

@retep998 Do you have an example of a function that you would expect not to be generated?

@retep998
Copy link
Member Author

retep998 commented Sep 5, 2016

@michaelwoerister For example a simple #[inline] pub fn foo() {}. Or perhaps a trait impl impl Clone for Foo { #[inline] fn clone(&self) -> Foo { *Foo } }. According to a quick test, doing --emit=llvm-ir with a rustc built with my patch, functions defined like that in winapi are still being generated.

@michaelwoerister
Copy link
Member

For example a simple #[inline] pub fn foo() {}.

If the function isn't used or referenced anywhere in the crate then the modification you made should have done the trick. I'll take a look when I find the time.

@michaelwoerister
Copy link
Member

I'm removing the nominated tag, this doesn't really need discussion in the meeting, it should just be investigated and fixed.

@michaelwoerister
Copy link
Member

While investigating #36309 I figured out why the fix suggested above didn't help: it only fixes the problem on the multiple-codegen-units-path. For it to work with just one codegen-unit, the change will be a bit more complicated, but it will probably be part of the fix for #36309.

bors added a commit that referenced this issue Sep 21, 2016
…r=nikomatsakis

trans: Only instantiate #[inline] functions in codegen units referencing them

This PR changes how `#[inline]` functions are translated. Before, there was one "master instance" of the function with `external` linkage and a number of on-demand instances with `available_externally` linkage in each codegen unit that referenced the function. This had two downsides:

* Public functions marked with `#[inline]` would be present in machine code of libraries unnecessarily (see #36280 for an example)
* LLVM would crash on `i686-pc-windows-msvc` due to what I suspect to be a bug in LLVM's Win32 exception handling code, because it doesn't like `available_externally` there (#36309).

This PR changes the behavior, so that there is no master instance and only on-demand instances with `internal` linkage. The downside of this is potential code-bloat if LLVM does not completely inline away the `internal` instances because then there'd be N instances of the function instead of 1. However, this can only become a problem when using more than one codegen unit per crate.

cc @rust-lang/compiler
@retep998
Copy link
Member Author

This was fixed by #36524

@Boddlnagg
Copy link
Contributor

Awesome! 🎉 This improved compile times of winrt-rust quite a lot:

time [secs] rlib size [bytes]
Before (Debug) 351.46 263.572.720
Before (Release) 506.68 154.972.432
After (Debug) 285.87 187.843.258
After (Release) 305.48 119.689.216

"Before" is rustc 1.13.0-nightly (c772948b6 2016-09-20) , "After" is rustc 1.13.0-nightly (4f9812a59 2016-09-21). See complete logs with -Z time-passes in this gist.

@brson
Copy link
Contributor

brson commented Sep 22, 2016

@Boddlnagg oh nice results!

@bluss
Copy link
Member

bluss commented Sep 23, 2016

ndarray (a much smaller crate) had compile time improve by 20% (with deps). Its release mode time for llvm passes dropped from 1.2 s to 0.2 s (just the crate itself).

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 11, 2020
…matthewjasper

Export `#[inline]` fns with extern indicators

In ancient history (rust-lang#36280) we stopped `#[inline]` fns being codegened if they weren't used. However,

- rust-lang#72944
- rust-lang#72463

observe that when writing something like

```rust
#![crate_type = "cdylib"]

#[no_mangle]
#[inline]
pub extern "C" fn foo() {
    // ...
}
```

we really _do_ want `foo` to be codegened. This change makes this the case.

Resolves rust-lang#72944, resolves rust-lang#72463 (and maybe some more)
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 16, 2020
…matthewjasper

Export `#[inline]` fns with extern indicators

In ancient history (rust-lang#36280) we stopped `#[inline]` fns being codegened if they weren't used. However,

- rust-lang#72944
- rust-lang#72463

observe that when writing something like

```rust
#![crate_type = "cdylib"]

#[no_mangle]
#[inline]
pub extern "C" fn foo() {
    // ...
}
```

we really _do_ want `foo` to be codegened. This change makes this the case.

Resolves rust-lang#72944, resolves rust-lang#72463 (and maybe some more)
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 18, 2020
…matthewjasper

Export `#[inline]` fns with extern indicators

In ancient history (rust-lang#36280) we stopped `#[inline]` fns being codegened if they weren't used. However,

- rust-lang#72944
- rust-lang#72463

observe that when writing something like

```rust
#![crate_type = "cdylib"]

#[no_mangle]
#[inline]
pub extern "C" fn foo() {
    // ...
}
```

we really _do_ want `foo` to be codegened. This change makes this the case.

Resolves rust-lang#72944, resolves rust-lang#72463 (and maybe some more)
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 18, 2020
…matthewjasper

Export `#[inline]` fns with extern indicators

In ancient history (rust-lang#36280) we stopped `#[inline]` fns being codegened if they weren't used. However,

- rust-lang#72944
- rust-lang#72463

observe that when writing something like

```rust
#![crate_type = "cdylib"]

#[no_mangle]
#[inline]
pub extern "C" fn foo() {
    // ...
}
```

we really _do_ want `foo` to be codegened. This change makes this the case.

Resolves rust-lang#72944, resolves rust-lang#72463 (and maybe some more)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants