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

rustc: Add inline(maybe), which only writes the AST to crate metadata. #6616

Closed
wants to merge 1 commit into from

Conversation

huonw
Copy link
Member

@huonw huonw commented May 19, 2013

This gives LLVM the data required to inline (i.e. writes the AST to the crate), but doesn't force it to, like #[inline] (which is actually just a hint, but is apparently very aggressive) or #[inline(always)].

I'm not sure how to test, but given:

// inline_maybe.rs
#[inline(maybe)]
pub fn foo() -> int { 1 }

// inline_maybe2.rs
extern mod inline_maybe;
use inline_maybe::foo;
fn main() {
    if foo() == 2 {
        print("hi");
    }
}

With a pre-inline(maybe) rustc the asm for inline_maybe2.rs contains:

    callq   _ZN3foo15_8faebfb4af26143_00E@PLT
    cmpq    $1, %rax
    jne .LBB0_4

With this patch, the foo call is inlined and constant-folded away. (with -O.)

With a huge foo (that wouldn't be normally inlined, even within the same crate) with inline(maybe), the actual call remains in inline_maybe2, i.e. it isn't inlined.

I'm not sure about two lines (my line comment), but I'm unsure who's most appropriate to consult/familiar with this code.

@bstrie
Copy link
Contributor

bstrie commented May 19, 2013

If #[inline] forces inlining, then how's that different from #[inline(always)] ?

@huonw
Copy link
Member Author

huonw commented May 19, 2013

#[inline] doesn't force inlining, but it's a very strong hint, i.e. (according to @thestinger) it almost always inlines, even when this isn't necessarily the best choice.

As far as I understand there are 4 inline settings for functions in LLVM: always, hint (our #[inline]), normal (no annotation) and never. For normal, LLVM can still decide to inline if it decides that it'd be better, but we don't provide a way for this to happen cross-crate (because we only write the necessary metadata for #[inline] and #[inline(always)]), that's what this PR is providing.

@brson
Copy link
Contributor

brson commented May 19, 2013

On first glance I'm a little uncomfortable with adding yet another type of inlining. Maybe we can solicit further feedback before committing.

@Aatch
Copy link
Contributor

Aatch commented May 19, 2013

There is definitely scope for it being useful, with LLVM's inlinehint being quite aggressive, the standard inlining heuristics might be preferable, but then you don't get cross-crate inlining.

The idea behind this is sound though.

@thestinger
Copy link
Contributor

@huonw: needs a rebase

@huonw
Copy link
Member Author

huonw commented May 27, 2013

@thestinger done!

@emberian
Copy link
Member

Hey I just met you, and this is crazy, but here's my AST, inline me maybe?

@brson
Copy link
Contributor

brson commented Jun 1, 2013

I prefer to not add #[inline(maybe)] at this time. Adding it later is a backwards compatible change.

#[inline(maybe)] only affects non-generic functions because generic ones are monomorphised.

An inline maybe function is guaranteed to be copied into every crate where it's used, even if not inlined. This is probably the right behavior since it's consistent - you know that no #[inline(maybe)] calls are into dynamic libraries - but I could imagine LLVM being smart enough to defer to the external function if it's not inlined.

When an #[inline(maybe)] function is copied but not inlined you do save some small amount, depending on platform, for not interacting with the dynamic linker.

I suppose that it is difficult to know when to choose #[inline(maybe)] over #[inline]. The right inline settings are hard for people to predict.

We don't have code that needs #[inline(maybe)] that I know of.

@brson
Copy link
Contributor

brson commented Jun 1, 2013

Closing. Feel free to reopen or revisit this later if you disagree.

@brson brson closed this Jun 1, 2013
@huonw huonw deleted the inline-maybe branch December 4, 2014 02:02
@huonw huonw restored the inline-maybe branch December 4, 2014 02:02
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 30, 2021
…ffen

New lint: exhaustive_enums, exhaustive_structs

Fixes rust-lang#6616

changelog: Added restriction lint: `exhaustive_enums`, `exhaustive_structs`
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.

6 participants