Suboptimal inlining decisions

[glandium]

I suspect this can happen in more cases, but here is how I observed this:

pub fn foo() -> Box<[u8]> {
    vec![0].into_boxed_slice()
}

This compiles to:

  sub rsp, 56
  lea rdx, [rsp + 8]
  mov edi, 1
  mov esi, 1
  call __rust_alloc@PLT
  test rax, rax
  je .LBB2_1
  mov byte ptr [rax], 0
  mov edx, 1
  add rsp, 56
  ret
.LBB2_1:
  (snip oom handling)

Which is pretty much to the point.

Now duplicate the function, so that you now have two functions calling into_boxed_slice(), and the compiler decides not to inline it at all anymore. Which:

• adds the full blown Vec::into_boxed_slice implementation (63 lines of assembly)
• adds ptr::drop_in_place
• and changes the function above to:

  sub rsp, 56
  lea rdx, [rsp + 8]
  mov edi, 1
  mov esi, 1
  call __rust_alloc@PLT
  test rax, rax
  je .LBB4_1
  mov byte ptr [rax], 0
  mov qword ptr [rsp + 8], rax
  mov qword ptr [rsp + 16], 1
  mov qword ptr [rsp + 24], 1
  lea rdi, [rsp + 8]
  call <alloc::vec::Vec<T>>::into_boxed_slice
  add rsp, 56
  ret
.LBB4_1:
  (snip oom handling)

The threshold to stop inlining seems pretty low for this particular case, and even if it might make sense for some uses across the codebase to not be inlined, when the result of inlining is clearly beneficial, it would be good if we could still inline the calls where it's a win.

[glandium]

Random thought: maybe this calls for an annotation to force-inline at the caller level.

[nox]

Cc @rust-lang/wg-codegen

[oli-obk]

I'd rather not consider more annotations. We already need to tell ppl that, yes there is #[inline(always)], no it will not make your code faster, it's a scalpel for edge cases.

We could probably more aggressively inline in the presence of constant information.

[nox]

Now duplicate the function, so that you now have two functions calling into_boxed_slice(), and the compiler decides not to inline it at all anymore.

I wonder how that interacts with #49479.

[hanna-kruppe]

MergeFunctions could help in the case where the entire callers are identical (edit:) and it runs before inlining. Which is not a very interesting case IMO for something like into_boxed_slice which probably is called from many different places.

I believe the root cause of this issue is that LLVM's inlining cost heuristic special cases functions with internal linkage and just one call site (because in those cases, inlining doesn't increase code size since you can eliminate the function entirely). If there are multiple call sites, IIRC it doesn't do anything to account for the fact that inlining all call sites would still allow eliminating the function. Probably because it can't actually know whether all call sites will inline it. Seems difficult to solve in general.

[oli-obk]

If inlining decreases the code size of the caller, we should always inline regardless of heuristics. Idk how to detect such cases before actually doing the inlining

[hanna-kruppe]

That too is part of the inlining heuristic. Of course, it's only an heuristic, and it could always be better, but tuning it further is notoriously fickle.

[nox]

All the functions involved in conversions between Vec<T> and Box<[T]> and between String and Box<str> should at least be annotated with #[inline], IMO. And for this purpose, shouldn't <Vec<T>>::shrink_to_fit be #[inline] and have its own self.capacity() != self.len check?

[glandium]

@oli-obk

I'd rather not consider more annotations. We already need to tell ppl that, yes there is #[inline(always)], no it will not make your code faster, it's a scalpel for edge cases.

The problem is that there is no scalpel for edge cases when the called function is not annotated at all, which the case here.

@rkruppe

If there are multiple call sites, IIRC it doesn't do anything to account for the fact that inlining all call sites would still allow eliminating the function. Probably because it can't actually know whether all call sites will inline it.

One way to look at it is that part of the problem is this all or nothing property of inlining. Either the function is always inlined or not (AIUI). I'd argue it should be decided case by case. There may be both cases where it makes sense for the function not to be inlined and cases where it doesn't, in the same codebase.

[hanna-kruppe]

@glandium You misunderstand, inlining in LLVM is a per-call-site decision (though it is true that most of the heuristic only looks at the function to inline, not at the call site).

[steveklabnik]

Triage; no change