Fix #[inline(always)] for recursive functions

[antoyo]

The following code:

#[inline(always)]
fn my_func(arg: i32) {
    if arg > 1 {
        my_func(arg - 1);
    }
}

fn main() {
    my_func(10);
}

triggers the following error:

libgccjit.so: error: : inlining failed in call to ‘always_inline’ ‘_ZN9test_rust7my_func17h0d9fc1667ee3d8a0E’: function not considered for inlining
libgccjit.so: src/main.rs:4:9: note: : called from here

As noted here, always_inline is not an optimization hint while #[inline(always)] is a hint as noted in the reference.

I'm not sure what the equivalent would be in GCC, though.

Maybe check what clang is doing (since it also has the issue) vs what rustc is doing.

#[rustc_force_inline] is possibly implemented via always_inline.

[FractalFir]

I have some ideas for a potential fix.

The very first question to answer is: can a non recursive function hit the inlining failed error?
In the newest version, the message mentions that the cause of the failure is recursive inlining.

libgccjit.so: error: : inlining failed in call to 'always_inline' 'fib.localalias': recursive inlining

This suggests there could be other causes - and that is something that needs to be checked.

Assuming that the only cause of this error is recursive inlining, the soultution is realtively straightforward.

Solutions

Ignore InlineAttr::Always

We could treat InlineAttr::Always the same way InlineAttr::Hint is treated, and translate it to FnAttribute::Inline.

However, that has obvious drawbacks: it will mean that we are not using the hints to their fullest, since there would be no difference between #[inline(always)] and #[inline]. This is undesirable, and may lead to worse codegen.

Trivial recursion check

We can't simply check if a function is calling itself, directly. That would catch simple cases of this problem, like this:

#[inline(always)]
#[unsafe(no_mangle)]
fn fib(n:u64)->u64{
    if n == 0 {return 1};
    fib(n - 1) + fib(n - 2)
}

However, we also need to worry about indirect recursion. In this example:

#[unsafe(no_mangle)]
fn fib(n:u64)->u64{
    if n == 0 {return 1};
    fib2(n - 1) + fib2(n - 2)
}
#[inline(always)]
#[unsafe(no_mangle)]
fn fib2(n:u64)->u64{
    if n == 0 {return 1};
    fib(n - 1) + fib(n - 2)
}

Neither fib nor fib2 is calling itself directly, but this will still trigger the bug.

Attribute-based check

Instead of checking if a function is calling itself, we can simply check if it is calling a function also marked with #[inline(always)]. We could then demote #[inline(always)] to ``#[inline]` in that case.

We could do this with a function like this:

/// 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.
fn maybe_resursively_inline<'gcc, 'tcx>(
    cx: &CodegenCx<'gcc, 'tcx>,
    instance: ty::Instance<'tcx>,
) -> bool {
    // No body, so we can't check if this is recursively inline, so we assume it is.
    if !cx.tcx.is_mir_available(instance.def_id()) {
        return true;
    }
    let body = cx.tcx.optimized_mir(instance.def_id());
    for block in body.basic_blocks.iter() {
        let Some(terminator) = &block.terminator else { continue };
        // I assume that the resursive-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.
        let TerminatorKind::Call { func, .. } = &terminator.kind else { continue };
        let Some((def, _args)) = func.const_fn_def() else { continue };
        // Check if the called function is also marked with `#[inline(always)]` .
        if matches!(
            cx.tcx.codegen_fn_attrs(def).inline,
            InlineAttr::Always | InlineAttr::Force { .. }
        ) {
            return true;
        }
    }
    false
}

This kind of check would then be only performed for functions marked with #[inline(always)].

/// Get GCC attribute for the provided inline heuristic, attached to `instance`.
#[cfg(feature = "master")]
#[inline]
fn inline_attr<'gcc, 'tcx>(
    cx: &CodegenCx<'gcc, 'tcx>,
    inline: InlineAttr,
    instance: ty::Instance<'tcx>,
) -> Option<FnAttribute<'gcc>> {
    match inline {
        InlineAttr::Always => {
            // We can't simply always return `always_inline` unconditionally.
            // It is *NOT A HINT* and does not work for recurisve functions.
            //
            // So, it can only be applied *if*:
            // The current function does not call any functions marked `#[inline(always)]`.
            //
            // That prevents issues steming from recursive `#[inline(always)]` at a *relatively* small cost.
            // We *only* need to check all the terminators of a function marked with this attribute.
            if maybe_resursively_inline(cx, instance) {
                Some(FnAttribute::Inline)
            } else {
                Some(FnAttribute::AlwaysInline)
            }
        }

This check is also relatively cheap, for a couple of reasons.

1. We are checking if a function is marked with #[inline(always)] during codegen: that means we will need a MIR body anyway, so the call to optimized_mir is effectively free(since the result is later used anyway).

2. We only iterate trough the terminators of a function, and there is usually a small(<50) number of them.

3. We only check the call terminators, where the operand is an FnDef. Since functions don't tend to call a lot of other functions, the cost of checking the attributes of the calle in this case should be neglegible in the grand scheme of things.

This solution is not without it's faults. It will cause:

#[inline(always)]
fn a(){}
#[inline(always)]
fn b(){
  a();
}

To be treated like:

#[inline(always)]
fn a(){}
#[inline]
fn b(){
  a();
}

Ideally, we would only need to demote #[inline(always)] in the cases where we know a recursive inline is in play.

Still, this relatively simple solution should solve this problem without much overhead or perf regressions.

[antoyo]

I really like your solution "Attribute-based check". I believe it could work, so we could try it out and see how it goes.