[go] cmd/compile: use staticuint64s for small constants
Gilbert Morgan (Gerrit)
Gilbert Morgan has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: use staticuint64s for small constants
When converting integer constants to interfaces, use pointers into
runtime.staticuint64s for values that fit in a byte (bools,
byte/int8/uint8, and other integers in the range [0, 255]).
This optimization reads the initialized readonly bytes of static
backing variables (stmps) during walk, replacing them with offsets
into runtime.staticuint64s.
Adopting and updating the work from CL 342131.
Updates #37612
Change diff
diff --git a/src/cmd/compile/internal/walk/convert.go b/src/cmd/compile/internal/walk/convert.go
index beef663..f652763 100644
--- a/src/cmd/compile/internal/walk/convert.go
+++ b/src/cmd/compile/internal/walk/convert.go
@@ -152,7 +152,12 @@
// Try a bunch of cases to avoid an allocation.
var value ir.Node
+ if v := smallIntFromLinksym(n); v != nil {
+ value = v
+ }
switch {
+ case value != nil:
+ // Already set.
case fromType.Size() == 0:
// n is zero-sized. Use zerobase.
diagnose("using global for zero-sized interface value", n)
@@ -244,6 +249,48 @@
return safeExpr(walkExpr(typecheck.Expr(call), init), init)
}
+// smallIntFromLinksym returns a LinksymOffsetExpr into staticuint64s if the
+// provided node is an stmp holding a value that fits in a single byte.
+func smallIntFromLinksym(n ir.Node) ir.Node {
+ if n.Op() != ir.ONAME || !n.Name().Readonly() {
+ return nil
+ }
+
+ fromType := n.Type()
+ size := fromType.Size()
+ if !fromType.IsBoolean() && !fromType.IsInteger() {
+ return nil
+ }
+
+ var v byte
+ p := n.(*ir.Name).Linksym().P
+
+ // Zero-length p is a zero value (and was left unassigned).
+ if len(p) != 0 {
+ var rest []byte
+ if ssagen.Arch.LinkArch.ByteOrder != binary.BigEndian {
+ v, rest = p[0], p[1:]
+ } else {
+ v, rest = p[size-1], p[:size-1]
+ }
+
+ // Ensure the other bytes are all zero, otherwise this
+ // integer is too big.
+ for _, b := range rest {
+ if b != 0 {
+ return nil
+ }
+ }
+ }
+
+ offset := int64(v) << 3
+ if ssagen.Arch.LinkArch.ByteOrder == binary.BigEndian {
+ offset += 8 - size
+ }
+
+ return ir.NewLinksymOffsetExpr(base.Pos, ir.Syms.Staticuint64s, offset, fromType)
+}
+
// walkBytesRunesToString walks an OBYTES2STR or ORUNES2STR node.
func walkBytesRunesToString(n *ir.ConvExpr, init *ir.Nodes) ir.Node {
a := typecheck.NodNil()
diff --git a/test/codegen/smallintiface.go b/test/codegen/smallintiface.go
index e1dbae5f..c22cd51 100644
--- a/test/codegen/smallintiface.go
+++ b/test/codegen/smallintiface.go
@@ -1,22 +1,103 @@
// asmcheck
-package codegen
-
// Copyright 2020 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
+package codegen
+
func booliface() interface{} {
- // amd64:`LEAQ runtime.staticuint64s\+8\(SB\)`
+ // 386:`LEAL\sruntime.staticuint64s\+8\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+8\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+15\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+15\(SB\),\sR\d+`
return true
}
func smallint8iface() interface{} {
- // amd64:`LEAQ runtime.staticuint64s\+2024\(SB\)`
+ // 386:`LEAL\sruntime.staticuint64s\+2024\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+2024\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+2031\(SB\)`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+2031\(SB\)`
return int8(-3)
}
func smalluint8iface() interface{} {
- // amd64:`LEAQ runtime.staticuint64s\+24\(SB\)`
+ // 386:`LEAL\sruntime.staticuint64s\+24\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+24\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+31\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+31\(SB\),\sR\d+`
return uint8(3)
}
+
+func smallintiface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+8\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+8\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+12\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+8\(SB\),\sR\d+`
+ return 1
+}
+
+func smallint16iface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+1016\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+1016\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+1022\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+1022\(SB\),\sR\d+`
+ return int16(127)
+}
+
+func smallint32iface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+2040\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+2040\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+2044\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+2044\(SB\),\sR\d+`
+ return int32(255)
+}
+
+func smallint64iface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+2040\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+2040\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+2040\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+2040\(SB\),\sR\d+`
+ return int64(255)
+}
+
+func smalluintface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+16\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+16\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+20\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+16\(SB\),\sR\d+`
+ return uint(2)
+}
+
+func smalluintptriface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+24\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+24\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+28\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+24\(SB\),\sR\d+`
+ return uintptr(3)
+}
+
+func smalluint16iface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+80\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+80\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+86\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+86\(SB\),\sR\d+`
+ return uint16(10)
+}
+
+func smalluint32iface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+8\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+8\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+12\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+12\(SB\),\sR\d+`
+ return uint32(1)
+}
+
+func smalluint64iface() interface{} {
+ // 386:`LEAL\sruntime.staticuint64s\+56\(SB\)`
+ // amd64:`LEAQ\sruntime.staticuint64s\+56\(SB\)`
+ // mips:`MOVW\s[$]runtime.staticuint64s\+56\(SB\),\sR\d+`
+ // mips64:`MOVV\s[$]runtime.staticuint64s\+56\(SB\),\sR\d+`
+ return uint64(7)
+}
Change information
- M src/cmd/compile/internal/walk/convert.go
- M test/codegen/smallintiface.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
staticuint64s := ir.NewLinksymExpr(base.Pos, ir.Syms.Staticuint64s, types.NewArray(types.Types[types.TUINT8], 256*8))We already use staticuint64s here. What's new about your code? (Is it the 16/32/64 bits cases?)
Related details
- Gilbert Morgan
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gilbert Morgan (Gerrit)
Gilbert Morgan added 1 comment![]( "Open in Gerrit")
staticuint64s := ir.NewLinksymExpr(base.Pos, ir.Syms.Staticuint64s, types.NewArray(types.Types[types.TUINT8], 256*8))We already use staticuint64s here. What's new about your code? (Is it the 16/32/64 bits cases?)
Hi Keith, yes, this optimization targets the 16, 32, and 64-bit integer types (e.g. int16, int64, uintptr, int).
Currently, the walk phase only optimizes single-byte types (bool, int8, uint8). This change extends it to larger integer types when their static backing data (stmps) fits within a single byte ([0, 255]), mapping them to offsets in runtime.staticuint64s.
I've added expanded codegen checks to test/codegen/smallintiface.go to assert this optimization for those larger types.
Related details
- Keith Randall
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gilbert Morgan (Gerrit)
Gilbert Morgan added 1 comment![]( "Open in Gerrit")
staticuint64s := ir.NewLinksymExpr(base.Pos, ir.Syms.Staticuint64s, types.NewArray(types.Types[types.TUINT8], 256*8))Gilbert MorganWe already use staticuint64s here. What's new about your code? (Is it the 16/32/64 bits cases?)
Hi Keith, yes, this optimization targets the 16, 32, and 64-bit integer types (e.g. int16, int64, uintptr, int).
Currently, the walk phase only optimizes single-byte types (bool, int8, uint8). This change extends it to larger integer types when their static backing data (stmps) fits within a single byte ([0, 255]), mapping them to offsets in runtime.staticuint64s.
I've added expanded codegen checks to test/codegen/smallintiface.go to assert this optimization for those larger types.
I'd appreciate your guidance on whether this is a reasonable approach or if there's a better way you'd recommend structuring it, or if I misinterpreted anything and it is in fact unneeded.
Gilbert Morgan (Gerrit)
Gilbert Morgan uploaded new patchset![]( "Open in Gerrit")
Related details
- Keith Randall
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gilbert Morgan (Gerrit)
Gilbert Morgan uploaded new patchset![]( "Open in Gerrit")
Related details
- Keith Randall
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 2 comments![]( "Open in Gerrit")
// and staticuint64s[n * 8 + 7] on big-endian.This code currently only works on 1-byte things because it handles non-constant args. We don't need to check the range of the argument.
For your change, you either need to handle only constants (and do the range check in the compiler), or have a runtime range check and fallback. (Like the code here: https://github.com/golang/go/blob/88a3e6202ae3ea8c5c60fb157ea645baaca67512/src/runtime/iface.go#L389)
staticuint64s := ir.NewLinksymExpr(base.Pos, ir.Syms.Staticuint64s, types.NewArray(types.Types[types.TUINT8], 256*8))Gilbert MorganWe already use staticuint64s here. What's new about your code? (Is it the 16/32/64 bits cases?)
Gilbert MorganHi Keith, yes, this optimization targets the 16, 32, and 64-bit integer types (e.g. int16, int64, uintptr, int).
Currently, the walk phase only optimizes single-byte types (bool, int8, uint8). This change extends it to larger integer types when their static backing data (stmps) fits within a single byte ([0, 255]), mapping them to offsets in runtime.staticuint64s.
I've added expanded codegen checks to test/codegen/smallintiface.go to assert this optimization for those larger types.
I'd appreciate your guidance on whether this is a reasonable approach or if there's a better way you'd recommend structuring it, or if I misinterpreted anything and it is in fact unneeded.
Ok, then I think we want to avoid having this handled in two different places, one for bool/uint8 and a different one for uint16/32/64. I'd rather it all be in just one place.
My slight preference would be to modify the existing code by removing the `fromType.Size() == 1` conditional and adding some range test instead.
This code can be tricky, for example using cheapExpr to ensure we don't evaluate the input arg more than once. (Although that may not be a problem if you're only handling constant literals.)
Related details
- Gilbert Morgan
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gilbert Morgan (Gerrit)
Gilbert Morgan uploaded new patchset![]( "Open in Gerrit")
Related details
- Gilbert Morgan
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gilbert Morgan (Gerrit)
Gilbert Morgan added 1 comment![]( "Open in Gerrit")
staticuint64s := ir.NewLinksymExpr(base.Pos, ir.Syms.Staticuint64s, types.NewArray(types.Types[types.TUINT8], 256*8))Gilbert MorganWe already use staticuint64s here. What's new about your code? (Is it the 16/32/64 bits cases?)
Gilbert MorganHi Keith, yes, this optimization targets the 16, 32, and 64-bit integer types (e.g. int16, int64, uintptr, int).
Currently, the walk phase only optimizes single-byte types (bool, int8, uint8). This change extends it to larger integer types when their static backing data (stmps) fits within a single byte ([0, 255]), mapping them to offsets in runtime.staticuint64s.
I've added expanded codegen checks to test/codegen/smallintiface.go to assert this optimization for those larger types.
Keith RandallI'd appreciate your guidance on whether this is a reasonable approach or if there's a better way you'd recommend structuring it, or if I misinterpreted anything and it is in fact unneeded.
Ok, then I think we want to avoid having this handled in two different places, one for bool/uint8 and a different one for uint16/32/64. I'd rather it all be in just one place.
My slight preference would be to modify the existing code by removing the `fromType.Size() == 1` conditional and adding some range test instead.
This code can be tricky, for example using cheapExpr to ensure we don't evaluate the input arg more than once. (Although that may not be a problem if you're only handling constant literals.)
I took a crack at it. Hopefully I picked up what you were putting down. I tested, benchmarked, and even model checked my logic. Do let me know if I should make any adjustments.
Related details
- Keith Randall
- Matthew Dempsky
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |