-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/compile: optimize code in the encoding/binary package #51724
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
Comments
That zig code is not equivalent because you're passing a [8]u8, while the Go snippet uses a slice, which we need to boundcheck. This Go Function:
generates with
Which is the same as the LLVM output. |
@ALTree, thank you for the clarification. I did not notice the problem, since in Zig it is possible to convert a slice to an array; as an example: var start: usize = 2;
var buf = [_]u8{ 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8 };
var b = buf[start..]; // A slice since start is not a constant
var bytes = b[0..8]; // An array since the indices are constants It should be possible(?) to implement this in Go, but it will break compatibility. |
In the Go version that takes a slice we have to boundcheck it, but if you remove the instructions related to that, it's just a single |
I wrote this test code: package test
import "encoding/binary"
func Parse(b []byte) uint64 {
v := binary.BigEndian.Uint64(b[0:8])
return v
}
func ParseUint64(b []byte, start, end int) uint64 {
v := binary.BigEndian.Uint64(b[start:end])
return v
}
func ParseUint64_2(b []byte, start, end int) uint64 {
v := binary.BigEndian.Uint64(b[start:end][0:8])
return v
} The first and last functions (where slicing is done using constants), generate a single I searched for real examples and found: The code can probably be improved with (not tested): binary.BigEndian.Uint16(buf[i*6:][:2]) Zig, as a counter example, will report a compiler error so it is more easy to detect the problem. NOTE: MOVZX 0(AX), AX
ROLW $0x8, AX while the Zig compiler generates movbe ax,WORD PTR [rdi] The source code is: package test
import "encoding/binary"
func Parse(b []byte) uint16 {
v := binary.BigEndian.Uint16(b[0:2])
return v
} Is there a reason for not using |
No particular reason, 16-bit versions just hasn't been implemented yet. |
Change https://go.dev/cl/395474 mentions this issue: |
Change https://go.dev/cl/396617 mentions this issue: |
This CL add MOVBE support for 16-bit version, but MOVBEWload is excluded because it does not satisfy zero extented. For #51724 Change-Id: I3fadf20bcbb9b423f6355e6a1e340107e8e621ac Reviewed-on: https://go-review.googlesource.com/c/go/+/396617 Reviewed-by: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Trust: Emmanuel Odeke <emmanuel@orijtech.com>
The
encoding/binary
package uses portable bit operations to read and write integers.Using the program:
and building with:
the resulting assembly is:
The Zig compiler, with the same code and ReleaseSafe build mode, is able to generate the assembly:
See https://godbolt.org/z/MPbE3P9j8.
Note that the
movbe
instruction was added in AMD64 v3 (https://en.wikipedia.org/wiki/X86-64).Zig currently uses the LLVM backend, so I don't know if the cost/complexity of this optimization is high.
The text was updated successfully, but these errors were encountered: