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: improve DCE when inlining empty functions #21440

Closed
josharian opened this issue Aug 14, 2017 · 5 comments
Closed

cmd/compile: improve DCE when inlining empty functions #21440

josharian opened this issue Aug 14, 2017 · 5 comments
Labels
FrozenDueToAge Performance
Milestone
Unplanned

Comments

@josharian
Copy link
Contributor 

package p

func f(e interface{}) {}

func g() {
	var s string
	f(s)
}

g compiles to:

"".g STEXT nosplit size=41 args=0x0 locals=0x18
	0x0000 00000 (y.go:5)	TEXT	"".g(SB), NOSPLIT, $24-0
	0x0000 00000 (y.go:5)	SUBQ	$24, SP
	0x0004 00004 (y.go:5)	MOVQ	BP, 16(SP)
	0x0009 00009 (y.go:5)	LEAQ	16(SP), BP
	0x000e 00014 (y.go:5)	FUNCDATA	$0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x000e 00014 (y.go:5)	FUNCDATA	$1, gclocals·f207267fbf96a0178e8758c6e3e0ce28(SB)
	0x000e 00014 (y.go:7)	MOVQ	$0, ""..autotmp_4(SP)
	0x0016 00022 (y.go:7)	MOVQ	$0, ""..autotmp_4+8(SP)
	0x001f 00031 (y.go:8)	MOVQ	16(SP), BP
	0x0024 00036 (y.go:8)	ADDQ	$24, SP
	0x0028 00040 (y.go:8)	RET

It should compile to nothing. In general, when inlining an empty function, all that should remain is evaluating its arguments for side-effects.

Related but slightly different: Replace the body of g with f("abc"). Then g compiles to nothing, but we still emit a vestigial "abc" string symbol.

This kind of code commonly shows up as a result of disabling features with build tags. See #21424.

cc @gstupp
@josharian josharian added this to the Unplanned milestone Aug 14, 2017
@martisch
Copy link
Contributor

partially related - reminds me of
cmd/compile: compiler generates calls to empty functions #4563
cmd/compile: inlining of empty functions computes arguments #4204

@randall77
Copy link
Contributor

Also related, issue #13095. It also requires the ability to get rid of autotmps that are assigned to but are otherwise unused.

@gstupp
Copy link

gstupp commented Aug 15, 2017

It would be great to be able to disable features with empty functions!

Please keep in mind that for a scenario as described in #21424 the statement given by @griesemer is really necessary because it allows me to depend on this functionality (in that specific case to define the scope of support to constant expressions, assert that this is the idiomatic way to solve the problem and guarantee future support).

@randall77
Copy link
Contributor

This is fixed by CL 38746.
Thanks @mundaym !

@josharian We still do emit the go.string."abc" symbol. Maybe open a different issue for that.
The string symbol will be removed by the linker so I'm not too worried about it.

@gopherbot
Copy link

Change https://golang.org/cl/58732 mentions this issue: cmd/compile,math: improve code generation for math.Abs

gopherbot pushed a commit that referenced this issue Aug 25, 2017
@randall77
cmd/compile,math: improve code generation for math.Abs
fb05948 
Implement int reg <-> fp reg moves on amd64.
If we see a load to int reg followed by an int->fp move, then we can just
load to the fp reg instead.  Same for stores.

math.Abs is now:

MOVQ	"".x+8(SP), AX
SHLQ	$1, AX
SHRQ	$1, AX
MOVQ	AX, "".~r1+16(SP)

math.Copysign is now:

MOVQ	"".x+8(SP), AX
SHLQ	$1, AX
SHRQ	$1, AX
MOVQ	"".y+16(SP), CX
SHRQ	$63, CX
SHLQ	$63, CX
ORQ	CX, AX
MOVQ	AX, "".~r2+24(SP)

math.Float64bits is now:

MOVSD	"".x+8(SP), X0
MOVSD	X0, "".~r1+16(SP)
(it would be nicer to use a non-SSE reg for this, nothing is perfect)

And due to the fix for #21440, the inlined version of these improve as well.

name      old time/op  new time/op  delta
Abs       1.38ns ± 5%  0.89ns ±10%  -35.54%  (p=0.000 n=10+10)
Copysign  1.56ns ± 7%  1.35ns ± 6%  -13.77%  (p=0.000 n=9+10)

Fixes #13095

Change-Id: Ibd7f2792412a6668608780b0688a77062e1f1499
Reviewed-on: https://go-review.googlesource.com/58732
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Ilya Tocar <ilya.tocar@intel.com>
@golang golang locked and limited conversation to collaborators Aug 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@josharian @martisch @randall77 @gopherbot @gstupp