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

Jump to bottom

cmd/compile: eliminate all (some?) convT2{I,E} calls #15375

Open
randall77 opened this issue Apr 19, 2016 · 5 comments
Open

cmd/compile: eliminate all (some?) convT2{I,E} calls #15375

randall77 opened this issue Apr 19, 2016 · 5 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unplanned

Comments

@randall77
Copy link
Contributor

For pointer-shaped types, convT2{I,E} are already done by the compiler. We currently call into the runtime only for non-pointer-shaped types.
There are two cases, where the interface escapes and where it doesn't.

type T struct {
    a, b, c int
}
func f(t T) interface{} {
    return t
}
func g(t T) {
    h(t)
}
//go:noescape
func h(interface{})

In both cases, I think, it would be easier to inline the work that convT2{I,E} does. f does this:

LEAQ    type."".T(SB), AX
MOVQ    AX, (SP)
LEAQ    "".autotmp_0+40(SP), AX
MOVQ    AX, 8(SP)
MOVQ    $0, 16(SP)
CALL    runtime.convT2E(SB)
MOVQ    24(SP), CX
MOVQ    32(SP), DX

instead it could do:

LEAQ    type."".T(SB), AX
MOVQ    AX, (SP)
CALL    runtime.newobject(SB)
MOVQ    8(SP), DX
MOVQ    "".autotmp_0+40(SP), AX
MOVQ    "".autotmp_0+48(SP), BX
MOVQ    "".autotmp_0+56(SP), CX
MOVQ    AX, (DX)
MOVQ    BX, 8(DX)
MOVQ    CX, 16(DX)
LEAQ    type."".T(SB), CX

11 instructions instead of 8, but several runtime calls bypassed (convT2E, plus the typedmemmove it calls, and everything that calls, ...).
The expanded code gets larger if T has a pointer in it. Maybe we use typedmemmove instead of explicit copy instructions in that case.

g is even easier because there is no runtime call required at all. Old code:

LEAQ    type."".T(SB), AX
MOVQ    AX, (SP)
LEAQ    "".autotmp_2+64(SP), AX
MOVQ    AX, 8(SP)
LEAQ    "".autotmp_3+40(SP), AX
MOVQ    AX, 16(SP)
CALL    runtime.convT2E(SB)
MOVQ    24(SP), CX
MOVQ    32(SP), DX

new code:

MOVQ    "".autotmp_2+64(SP), AX
MOVQ    "".autotmp_2+72(SP), BX
MOVQ    "".autotmp_2+80(SP), CX
MOVQ    AX, "".autotmp_3+40(SP)
MOVQ    BX, "".autotmp_3+48(SP)
MOVQ    CX, "".autotmp_3+56(SP)
LEAQ    type."".T(SB), CX
LEAQ    "".autotmp_3+40(SP), DX

one less instruction, and if we can avoid the copy (if autotmp_2 is never modified after), it could be even better. And no write barriers are required on the copy even if T has pointers.

Maybe we do the no-escape optimization first. It seems like an obvious win. That would allow removing the third arg to convT2{I,E}. Then we could think about the escape optimization.

@walken-google
@randall77 randall77 added this to the Go1.8 milestone Apr 19, 2016
@randall77 randall77 mentioned this issue May 3, 2016
Closed
@randall77 randall77 mentioned this issue Sep 17, 2016
Closed
@gopherbot
Copy link

CL https://golang.org/cl/29373 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 19, 2016
@randall77
cmd/compile: inline convT2{I,E} when result doesn't escape
6129f37 
No point in calling a function when we can build the interface
using a known type (or itab) and the address of a local.

Get rid of third arg (preallocated stack space) to convT2{I,E}.

Makes go binary smaller by 0.2%

benchmark                   old ns/op     new ns/op     delta
BenchmarkEfaceInteger-8     16.7          10.1          -39.52%

Update #17118
Update #15375

Change-Id: I9724a1f802bfa1e3957bf1856b55558278e198a2
Reviewed-on: https://go-review.googlesource.com/29373
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@randall77 randall77 mentioned this issue Sep 20, 2016
Closed
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 11, 2016
@rsc
Copy link
Contributor

rsc commented Oct 21, 2016

Sounds like the escape case is for later.

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 21, 2016
@bradfitz bradfitz modified the milestones: Go1.9Early, Go1.10Early May 3, 2017
@bradfitz bradfitz added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017
@griesemer griesemer modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@griesemer
Copy link
Contributor

Seems too late now. Moving to 1.11.

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12, Unplanned May 18, 2018
@josharian
Copy link
Contributor

@randall77 I think this may be done. Do you see anything left to do here?

@randall77
Copy link
Contributor Author

Yes, I think there's more to be done. The escape case (f in my example) seems nonoptimal. It makes a temporary, then passes the address of that temporary to convT2Enoptr.
We could just call newobject directly, then initialize its contents in compiled code. I don't think it would be any bigger (for pointerless types, anyway).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@bradfitz @josharian @rsc @quentinmit @randall77 @griesemer @gopherbot