-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: combine small loads #14267
Comments
Is this a duplicate of #11819? |
I think we must recognize both high to low and low to high index order as
otherwise it's impossible to write code that perform equally well on both
endianness.
|
@minux, I don't see the relationship between memory access order and endianness; you can load from higher or lower address first regardless of the endianness. Unless you mean for code already in production with compatibility agreements? As in the case of encoding/binary.PutUint64, where the indexes already start from a lower address, can't we do an initial bounds check that the length is >= 8? If yes, combine the stores, otherwise, fall back the original code. That is, the compiler will implicitly convert the code to be: func (bigEndian) PutUint64(b []byte, v uint64) {
if len(b) >= 8 {
// Combined store version of the code below.
return
}
b[0] = byte(v >> 56)
b[1] = byte(v >> 48)
b[2] = byte(v >> 40)
b[3] = byte(v >> 32)
b[4] = byte(v >> 24)
b[5] = byte(v >> 16)
b[6] = byte(v >> 8)
b[7] = byte(v)
} This, ensures that we preserve the behavior of partial stores, but remains fast in the vastly-common case. |
CL https://golang.org/cl/20218 mentions this issue. |
This only deals with the loads themselves. The bounds checks are a separate issue. Also doesn't handle stores, those are harder because we need to make sure intermediate memory states aren't observed (which is hard to do with rewrite rules). Use one byte shorter instructions for zero-extending loads. Update #14267 Change-Id: I40af25ab5208488151ba7db32bf96081878fa7d9 Reviewed-on: https://go-review.googlesource.com/20218 Reviewed-by: Alexandru Moșoi <[email protected]> Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]>
CL https://golang.org/cl/22326 mentions this issue. |
encoding/binary/binary.go:
It would be nice to combine the small loads into one larger load, at least for architectures with unaligned loads.
See #14266 for the bug about combining the bounds checks. This bug is about combining the loads themselves. We could use rewrite rules something like:
(Or64 (Load <t> ptr mem) (Shl64x64 (Load <t> (Add64 ptr (Const64 [t.Size()])) mem) (Const64 [8*t.Size()]))) -> (Load ptr mem)
These rules would probably work independent of whether the bounds checks are fixed or not.
Maybe we could do stores also. That would require the bounds checks to go first, and the user to write to the max index first (otherwise partial writes would be observable).
The text was updated successfully, but these errors were encountered: