[goal] modifying private method in another crate's impl should not require users of public methods to recompile

[nikomatsakis]

This is an extension of #37121 but to support modification of a private method in another crate. This is currently complicated by the fact that when we write metadata, we make one metadata node per item. So any changes to the item at all are confounded together. IOW, we can't tell the difference between adding an inherent impl or method to a struct and changing the types of its fields. This results in really poor re-use.

[nikomatsakis]

cc @michaelwoerister

[nikomatsakis]

As far as priorities, I would put this goal somewhat secondary -- it's very important, but we should focus first on #37121 and of course correctness goals. IOW we can have a widely used version that doesn't handle this yet, since most people work primarily at a leaf crate when building. But we should link this goal perhaps to some issues that pertain to specific impl changes for fixing it.

[nikomatsakis]

triage: P-medium

[nagisa]

The private callees might have been inlined through the public method.

[KalitaAlexey]

@nagisa Good note. Is there any way to determine whether some private item was inlined? We must investigate this case.

[michaelwoerister]

Inlining should mostly be handled by splitting interfaces from bodies for Metadata dep-nodes, like we also plan to do for HIR dep-nodes.

1. If the public extern method is neither generic nor marked with #[inline] then we should not need to care what got inlined into it.
2. If the public extern method is generic or marked with #[inline], we must track what goes into its body. If inlining happens at the MIR level, this should already be covered by the current tracking system. At LLVM level it's more difficult. I could imagine that we give the Rust compiler a heuristic that decides which functions are very unlikely to be inlined and then mark those functions with #[inline(never)] at the LLVM level --- and add dependency edges to all other callees because we can't guarantee that they haven't been inlined.

[hanna-kruppe]

Just because a function (in the same LLVM module) is noinline doesn't mean LLVM won't peek at its implementation. For example, consider this playground example:

#[inline(never)]
pub fn five() -> i32 {
    5
}

pub fn caller() -> i32 {
    five() + 1
}

The LLVM IR for five is noinline (in debug and in release), but in release mode, caller gets optimized to ret i32 6. It also adds various attributes to caller that can only be justified by analyzing or inlining five (e.g., nounwind).

[michaelwoerister]

@rkruppe Wow! That's a pretty blatant violation of what the documentation says about the attribute. Thanks for the info!

Oh well, we can still fall back to the brute force method of preventing inlining by moving those functions into their own compilation unit.

[nagisa]

Wow! That's a pretty blatant violation of what the documentation says about the attribute. Thanks for the info!

This optimisation happens because of global (cross-function) constant propagation and not inlining, so no violations here. The way I see it, this goal cannot be achieved without instrumenting LLVM appropriately.

As an aside, LLVM has quite a few module-wide optimisations (inlining and global constprop being two examples), all of which would have to be instrumented somehow. Separate compilation units sound quite inefficient to me at the first consideration.

[eddyb]

Separate compilation units might work with ThinLTO, if we get the granularity just right.

[michaelwoerister]

Separate compilation units sound quite inefficient to me at the first consideration.

They are probably not ideal from a compile-time point of view.

One consequence of instrumenting LLVM would be that we have to push some of the dependency tracking past the LLVM passes. Probably not that big a deal.

A short-term solution would be to add dependency edges to everything that is transitively called within the same compilation unit (i.e. the worst case result that LLVM instrumentation data could yield).

[michaelwoerister]

Oh wait, all of this might be a red herring: We are never re-using LLVM IR from an external crate. And regenerating the IR for locally inlined instances will necessarily create dependency edges to the MIR in metadata, so we should be fine.

[nikomatsakis]

Just because a function (in the same LLVM module) is noinline doesn't mean LLVM won't peek at its implementation.

Right, this is one of the reasons we don't attempt to second guess LLVM's inlining, instead relying on just not giving it access to data, and assuming it uses whatever we give it, no matter what we say....

Oh wait, all of this might be a red herring

...and yes, I'm not sure how all of this is relevant. =)

Certainly if the function is #[inline] and we regenerate its IR, that is one thing...but that wasn't the case I was worried about, really. (And even then, we should be able to limit the effect of its propagation in many cases.)

[Aaron1011]

I'd like to work on this - is there a good place that I should start?

[michaelwoerister]

This probably already works. So I suggest:

1. See if there's a test case for it already in src/test/incremental.
2. If not, add one.

[michalt]

With the test added, I think this can be closed now. :)

[michaelwoerister]

Yes, the test looks good. Thanks, @michalt!