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

cmd/compile: use staticuint64s #37612

Open
josharian opened this issue Mar 2, 2020 · 16 comments
Open

cmd/compile: use staticuint64s #37612

josharian opened this issue Mar 2, 2020 · 16 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Mar 2, 2020

CL 216401 introduced a new static array of uint64s running from 0 to 255.

Two follow-ups here:

@josharian josharian added this to the Go1.15 milestone Mar 2, 2020
@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Mar 2, 2020

This may be a decent starter issue for someone wanting to work on the compiler.

@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Mar 3, 2020

I'm not sure what this has to do with the compiler? It seems to be purely a runtime optimization - the linked CL doesn't touch cmd/compile at all.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Mar 3, 2020

Look for staticbytes in cmd/compile/internal/gc.

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Mar 3, 2020

Right. But now that we have staticuint64s in the runtime, we can use it in the compiler.

For part one, grep the compiler for staticbytes and use staticuint64s instead. (The offset math will need to change a bit; see Keith's linked CL comment.) Then delete runtime.staticbytes.

For part two, in places where we only optimize byte-sized things, we can probably now optimize constant larger-int-sized things. The sites will be the same sites encountered in part one.

@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Mar 3, 2020

There are actually two more remaining uses of runtime.staticbytes, in src/runtime/string.go, for generating 1-character strings.

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Mar 3, 2020

Thanks! Those could also be converted, using similar arithmetic.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 4, 2020

Change https://golang.org/cl/221957 mentions this issue: cmd/compile: use staticuint64s instead of staticbytes

@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Mar 4, 2020

I've sent a CL implementing the first part of the compiler change. I'll do the other two follow-ups later.

gopherbot pushed a commit that referenced this issue Mar 4, 2020
There are still two places in src/runtime/string.go that use
staticbytes, so we cannot delete it just yet.

There is a new codegen test to verify that the index calculation
is constant-folded, at least on amd64. ppc64, mips[64] and s390x
cannot currently do that.

There is also a new runtime benchmark to ensure that this does not
slow down performance (tested against parent commit):

name                      old time/op  new time/op  delta
ConvT2EByteSized/bool-4   1.07ns ± 1%  1.07ns ± 1%   ~     (p=0.060 n=14+15)
ConvT2EByteSized/uint8-4  1.06ns ± 1%  1.07ns ± 1%   ~     (p=0.095 n=14+15)

Updates #37612

Change-Id: I5ec30738edaa48cda78dfab4a78e24a32fa7fd6a
Reviewed-on: https://go-review.googlesource.com/c/go/+/221957
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 5, 2020

Change https://golang.org/cl/221979 mentions this issue: runtime: use staticuint64s instead of staticbytes for 1-length strings

gopherbot pushed a commit that referenced this issue Mar 5, 2020
This was the last remaining use of staticbytes, so we can now
delete it.

The new code appears slightly faster on amd64:

name                   old time/op  new time/op  delta
SliceByteToString/1-4  6.29ns ± 2%  5.89ns ± 1%  -6.46%  (p=0.000 n=14+14)

This may not be the case on the big-endian architectures, since they have
to do an extra addition.

Updates #37612

Change-Id: Icb84c5911ba025f798de152849992a55be99e4f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/221979
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Mar 13, 2020

I was trying to do the small int constant part, but by the time walkexpr runs, the OLITERAL has been replaced with an ONAME node for a temporary:

before walk smallintiface
.   RETURN l(26) tc(1)
.   RETURN-list
.   .   CONVIFACE l(26) esc(h) tc(1) implicit(true) INTER-@0
.   .   .   NAME-codegen..stmp_3 l(26) x(0) class(PEXTERN) tc(1) used int

I can't figure out how to recover the value of the constant from that ONAME.

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Mar 13, 2020

You’ll probably need to make a companion change to order.go at the same time. I’m AFK for a little while. If that isn’t enough of a hint, let me know and I’ll expand on it when I am at a real keyboard.

@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Mar 14, 2020

I tried narrowing the condition in the OCONVIFACE case of (*Order).expr as follows:

// Don't use a temporary for integer constants that fit in a byte; walk will replace
// them with pointers into runtime.staticuint64s.
if _, needsaddr := convFuncName(n.Left.Type, n.Type); (needsaddr || isStaticCompositeLiteral(n.Left)) && !(n.Left.CanInt64() && n.Left.Int64() >= 0 && n.Left.Int64() < 256) {

That almost works, but I get a couple of test failures, which seem directly relevant to this code:

# go run run.go -- writebarrier.go

Unmatched Errors:
writebarrier.go:288: write barrier

FAIL	writebarrier.go	0.092s
# go run run.go -- fixedbugs/issue31782.go
exit status 1
panic: interface conversion: interface {} is nil, not int

goroutine 1 [running]:
main.main()
	/Users/dpinela/dev/golang-dev/go/test/fixedbugs/issue31782.go:23 +0x78
exit status 2

FAIL	fixedbugs/issue31782.go	0.308s

I've had a look at the code in order.go and sinit.go that handles composite literals, but I'm having a hard time following what's going on, so I'm stuck.

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Mar 14, 2020

Please post your changes in a CL so I can take a look. Thanks.

I’m still AFK, and due to coronavirus school closures, it is unclear when I will next have meaningful laptop time. But I will look as best as I can, next opportunity I get.

@josharian

This comment has been minimized.

Copy link
Contributor Author

@josharian josharian commented Mar 14, 2020

I wonder—have you checked what code we generate right now for this case? I wonder whether we have enough existing optimizations at tip (probably not 1.14) to already optimally handle the case of a small constant int. (There’s an entirely separate question of handling an int that is non-constant but whose unsigned equivalent provably fits in one byte. That we definitely don’t handle well now. But I don’t think that that is what you were looking at, either.)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 14, 2020

Change https://golang.org/cl/223358 mentions this issue: cmd/compile: use staticuint64s to optimize interface conversion of small integer constants (WIP)

@dpinela

This comment has been minimized.

Copy link
Contributor

@dpinela dpinela commented Mar 14, 2020

Currently (both 1.14 and tip) we generate a static global with the constant's value and use a pointer to that, like we do for string literals:

"".X STEXT nosplit size=25 args=0x10 locals=0x0
	0x0000 00000 (smallintiface.go:3)	TEXT	"".X(SB), NOSPLIT|ABIInternal, $0-16
	0x0000 00000 (smallintiface.go:3)	PCDATA	$0, $-2
	0x0000 00000 (smallintiface.go:3)	PCDATA	$1, $-2
	0x0000 00000 (smallintiface.go:3)	FUNCDATA	$0, gclocals·568470801006e5c0dc3947ea998fe279(SB)
	0x0000 00000 (smallintiface.go:3)	FUNCDATA	$1, gclocals·69c1753bd5f81501d95132d08af04464(SB)
	0x0000 00000 (smallintiface.go:3)	FUNCDATA	$2, gclocals·9fb7f0986f647f17cb53dda1484e0f7a(SB)
	0x0000 00000 (smallintiface.go:4)	PCDATA	$0, $1
	0x0000 00000 (smallintiface.go:4)	PCDATA	$1, $1
	0x0000 00000 (smallintiface.go:4)	LEAQ	type.int(SB), AX
	0x0007 00007 (smallintiface.go:4)	PCDATA	$0, $0
	0x0007 00007 (smallintiface.go:4)	MOVQ	AX, "".~r0+8(SP)
	0x000c 00012 (smallintiface.go:4)	PCDATA	$0, $1
	0x000c 00012 (smallintiface.go:4)	LEAQ	""..stmp_0(SB), AX
	0x0013 00019 (smallintiface.go:4)	PCDATA	$0, $0
	0x0013 00019 (smallintiface.go:4)	MOVQ	AX, "".~r0+16(SP)
	0x0018 00024 (smallintiface.go:4)	RET
[...]
""..stmp_0 SRODATA size=8
	0x0000 01 00 00 00 00 00 00 00                          ........

This is optimal in execution time, but not in binary size: pointing into staticuint64s would allow us to elide these globals, saving 8 bytes each in the common case of int constants on 64-bit architectures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.