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

Move from #[inline(always)] to #[inline] #306

Merged
merged 3 commits into from
Jan 29, 2018

Commits on Jan 29, 2018

  1. Move from #[inline(always)] to #[inline]

    This commit blanket changes all `#[inline(always)]` annotations to `#[inline]`.
    Fear not though, this should not be a regression! To clarify, though, this
    change is done out of correctness to ensure that we don't hit stray LLVM errors.
    
    Most of the LLVM intrinsics and various LLVM functions we actually lower down to
    only work correctly if they are invoked from a function with an appropriate
    target feature set. For example if we were to out-of-the-blue invoke an AVX
    intrinsic then we get a [codegen error][avx-error]. This error comes about
    because the surrounding function isn't enabling the AVX feature. Now in general
    we don't have a lot of control over how this crate is consumed by downstream
    crates. It'd be a pretty bad mistake if all mistakes showed up as scary
    un-debuggable codegen errors in LLVM!
    
    On the other side of this issue *we* as the invokers of these intrinsics are
    "doing the right thing". All our functions in this crate are tagged
    appropriately with target features to be codegen'd correctly. Indeed we have
    plenty of tests asserting that we can codegen everything across multiple
    platforms!
    
    The error comes about here because of precisely the `#[inline(always)]`
    attribute. Typically LLVM *won't* inline functions across target feature sets.
    For example if you have a normal function which calls a function that enables
    AVX2, then the target, no matter how small, won't be inlined into the caller.
    This is done for correctness (register preserving and all that) but is also how
    these codegen errors are prevented in practice.
    
    Now we as stdsimd, however, are currently tagging all functions with "always
    inline this, no matter what". That ends up, apparently, bypassing the logic of
    "is this even possible to inline". In turn we start inlining things like AVX
    intrinsics into functions that can't actually call AVX intrinsics, creating
    codegen errors at compile time.
    
    So with all that motivation, this commit switches to the normal inline hints for
    these functions, just `#[inline]`, instead of `#[inline(always)]`. Now for the
    stdsimd crate it is absolutely critical that all functions are inlined to have
    good performance. Using `#[inline]`, however, shouldn't hamper that!
    
    The compiler will recognize the `#[inline]` attribute and make sure that each of
    these functions is *candidate* to being inlined into any and all downstream
    codegen units. (aka if we were missing `#[inline]` then LLVM wouldn't even know
    the definition to inline most of the time). After that, though, we're relying on
    LLVM to naturally inline these functions as opposed to forcing it to do so.
    Typically, however, these intrinsics are one-liners and are trivially
    inlineable, so I'd imagine that LLVM will go ahead and inline everything all
    over the place.
    
    All in all this change is brought about by rust-lang#253 which noticed various codegen
    errors. I originally thought it was due to ABI issues but turned out to be
    wrong! (although that was also a bug which has since been resolved). In any case
    after this change I was able to get the example in rust-lang#253 to execute in both
    release and debug mode.
    
    Closes rust-lang#253
    
    [avx-error]: https://play.rust-lang.org/?gist=50cb08f1e2242e22109a6d69318bd112&version=nightly
    alexcrichton committed Jan 29, 2018
    Configuration menu
    Copy the full SHA
    05a0df3 View commit details
    Browse the repository at this point in the history
  2. Add inline(always) on eflags intrinsics

    Their ABI actually relies on it!
    alexcrichton committed Jan 29, 2018
    Configuration menu
    Copy the full SHA
    e923961 View commit details
    Browse the repository at this point in the history
  3. Leave #[inline(always)] on portable types

    They're causing test failures on ARM, let's investigate later.
    alexcrichton committed Jan 29, 2018
    Configuration menu
    Copy the full SHA
    2ee6948 View commit details
    Browse the repository at this point in the history