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

Suboptimal invoke-related codegen for default trait impls #43150

Closed
alexcrichton opened this issue Jul 10, 2017 · 8 comments
Closed

Suboptimal invoke-related codegen for default trait impls #43150

alexcrichton opened this issue Jul 10, 2017 · 8 comments
Labels
A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

It looks like default methods in traits will always use invoke whereas defined methods will hit the optimization to not use invoke:

// bar.rs
#![crate_type = "rlib"]
pub fn bar() {}
// foo.rs
#![crate_type = "rlib"]
extern crate bar;

pub trait A: Sized {
    fn foo1(self) {
        bar::bar();
    }
    fn foo2(self);
}

impl A for i32 {
    fn foo2(self) {
        bar::bar();
    }
}

fn main() {
    0i32.foo1();
    0i32.foo2();
}
$ rustc bar.rs
$ rustc foo.rs -L . --emit llvm-ir
$ cat foo.ll

will yield this IR, notably:

; foo::A::foo1
; Function Attrs: uwtable
define internal void @_ZN3foo1A4foo117hcfa03453b669bd94E(i32) unnamed_addr #0 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
start:
  %personalityslot = alloca { i8*, i32 }
; invoke bar::bar
  invoke void @_ZN3bar3bar17h21868f3a4452d05bE()
          to label %bb3 unwind label %cleanup

; ...
}


; <i32 as foo::A>::foo2
; Function Attrs: uwtable
define void @"_ZN30_$LT$i32$u20$as$u20$foo..A$GT$4foo217h68eec0bc305998eeE"(i32) unnamed_addr #0 {
start:
; call bar::bar
  call void @_ZN3bar3bar17h21868f3a4452d05bE()
  br label %bb1

bb1:                                              ; preds = %start
  ret void
}

Note that <i32 as A>::foo1 uses an invoke instruction whereas <i32 as A>::foo2 does not.

@alexcrichton alexcrichton added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 10, 2017
@alexcrichton alexcrichton added O-windows-gnu Toolchain: GNU, Operating system: Windows and removed O-windows-gnu Toolchain: GNU, Operating system: Windows labels Jul 10, 2017
@est31
Copy link
Member

est31 commented Jul 10, 2017

Related: #40254 and 8f581cc

@arielb1
Copy link
Contributor

arielb1 commented Jul 11, 2017

This happens because we don't run MIR optimizations after monomorphization. In the "default trait impl" case, we don't know that Self: Copy, so we have to emit a landing pad that contains a destructor for Self, while in the "fn" case we do know that u32: Copy and don't emit a landing pad with a destructor call.

trans actually knows this landing pad is a no-op, but trans does not, and probably should not, perform that sort of optimization.

@arielb1 arielb1 closed this as completed Jul 11, 2017
@alexcrichton
Copy link
Member Author

@arielb1 I'm a little confused, is this not a bug? I'd personally at least consider this a compile time performance issue? We've had a lot of issues in the past with invoke instructions taking much longer to codege in LLVM than call instructions.

@arielb1
Copy link
Contributor

arielb1 commented Jul 12, 2017

I'm a little confused, is this not a bug? I'd personally at least consider this a compile time performance issue? We've had a lot of issues in the past with invoke instructions taking much longer to codege in LLVM than call instructions.

The "bug" is that we are not doing MIR optimizations on monomorphized code. This is far from the only way that can cause LLVM slowness. The LLVM problem on MSVC is indeed a (different) bug.

@alexcrichton
Copy link
Member Author

@arielb1 is there another bug to reference here? This behavior is actively causing bugs so I just want to make sure we don't forget about this.

@alexcrichton
Copy link
Member Author

ping @arielb1, just wanted to make sure this wasn't lost, is there a way to make sure we don't forget about this?

@arielb1
Copy link
Contributor

arielb1 commented Nov 23, 2017

@alexcrichton

i can't imagine us implementing post-monomorphization MIR optimizations in a way that does not fix this, and I find it hard to imagine us special-case-fixing this.

@mati865
Copy link
Contributor

mati865 commented Jan 8, 2019

i can't imagine us implementing post-monomorphization MIR optimizations in a way that does not fix this, and I find it hard to imagine us special-case-fixing this.

Is there tracking issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants