-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Support whole program devirtualization #68262
Comments
In LLVM terms, this means emitting type metadata and using |
I'd be interested to look into this issue. Since I'm not that familiar with this part of the compiler (but want to get more familiar with it) I first wanted to ask what would be involved in fixing this. AFAICT to fix this, a flag would have to be added to the compiler and if that flag is set, (some) types would have to be marked with the
@tmandry could you give me some pointers where to start with this issue? Do you have an estimate how much work would be involved here? |
This is not correct. A trait marked as invisible or restricted can still used by a foreign codegen unit or a foreign shared library. For example if it is used by a public generic or |
Ah right of course, so it has to depend on the visibility of the trait and its implementors. So if the trait is public, or if there are any generics involved or blanket impls exist, than it should be marked as |
It looks like we're already getting some form of this for free. The following code: trait Foo {
fn my_method(&self) {}
}
impl Foo for bool {
fn my_method(&self) {
std::process::abort();
}
}
#[inline(never)]
pub fn use_it(val: &dyn Foo) {
val.my_method();
}
pub fn main() {
use_it(&true);
} results in the following optimized LLVM IR for
|
This only happens when |
Using the code you shared, I still see a vtable in the final binary: https://rust.godbolt.org/z/qzb6o9MKG . Were you using a different version of Rust, or different set of compilation flags to achieve this? |
FYI, I have a semi-working version here: https://github.com/flip1995/rust/tree/pk-vfe It seems to still remove a bit too many functions or possibly does something wrong when converting the If the above example is compiled as a
|
@hudson-ayers I didn't scroll down far enough in the playground output. While the function is no longer making a call through the vtable, the vtable itself is still present in the binary, as you noticed. |
Is it correct this was closed by #96285 ? While dead virtual function elimination is awesome, it is not the same as whole program devirtualization, which can replace actually-called virtual functions with static calls. Supporting the first is not equivalent to supporting the second. And if |
This shouldn't have been closed, because it isn't quite finished yet. IIRC (it has been some time since I actively worked on this) it only enables VFE, not WPD. Though WPD is enabled by default (for LTO at least) and VFE is only an extension of the WPD optimization. |
For what is missing, see https://github.com/flip1995/rust/blob/195f2082002c9db456e0fde8c1d5db79929ae293/src/doc/unstable-book/src/compiler-flags/virtual-function-elimination.md and soon the unstable book. |
Is this dead in the water then? |
I don't think anyone is working on this right now. But in theory it should be possible to enable this optimization in |
When doing LTO, LLVM supports a devirtualization pass that converts some indirect calls to static, inlineable calls. This can unlock a lot of optimizations when dynamic dispatch is used frequently in a program.
I don't know how hard this would be to support in Rust. Clang has a flag
-fwhole-program-vtables
which we would need to emulate. From there it seems like it might "just work". I'm opening this issue to start collecting more information.I also found #11915 which mentions this, but not sure if any of the information there is still accurate.
The text was updated successfully, but these errors were encountered: