[go] cmd/compile: elide redundant sub-word extensions on arm64
Frank Kuehnel (Gerrit)
Frank Kuehnel has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: elide redundant sub-word extensions on arm64
On arm64 a MOVBload or MOVHload already produces a correctly sign- or
zero-extended value, so an extension applied to its result is redundant.
The "don't extend after proper load" rules in ARM64latelower.rules
recognise this, but rewrite the extension to MOVDreg, which is a real
register move. MOVDreg only becomes free via
(MOVDreg x) && x.Uses == 1 => (MOVDnop x)
that is, when the loaded value has exactly one use.
A loop that both compares and stores the loaded value gives it two uses,
so the demotion does not fire and a register-to-register MOVD is emitted
every iteration:
MOVB (R0), R3
MOVB R3, (R2)
MOVD R3, R2 // redundant
CMPW R1, R2
32- and 64-bit element types are unaffected, because a 32- or 64-bit
compare consumes the load's result directly and no extension node is ever
created.
Drop the extension node outright rather than turning it into a move. The
same file already does this for other redundant extensions, for instance
(MOVWUreg x) && ZeroUpper32Bits(x, 3) => x, and it is the direction the
existing "TODO: we should be able to get rid of MOVDnop all together"
points at.
Removing the use-count gate instead does not work: MOVDnop is
resultInArg0, so with two live uses the register allocator materialises
the copy anyway and the move survives, merely relocated.
Measured on a c4a-standard-8 (Google Axion, Neoverse-V2), pinned to an
isolated core, GOMAXPROCS=1, 12 rounds after a discarded warmup, with an
A/A control that showed no significant difference. Sorting 100k elements
with a branchless Hoare-Lomuto partition:
int8 -5.7%
int16 -3.5%
int32 ~
int64 +0.1%
An earlier run patching only the 8-bit rules left int16 at -0.1%, which
confirms the int16 result comes from the MOVHreg rules rather than drift.
slices.Sort does not speed up at int8 (+0.1%) even though its partition
loses the same two instructions -- 178 to 175, with reg-to-reg moves
going 5 to 3. That loop is a branchy Hoare scan dominated by branch
mispredictions, so two instructions are not visible in it. A branchless
loop's instruction count is its cost, which is where the gain above comes
from.
Runs of slices.Sort at int32 and int64 moved -2.6% and -2.3%, but
partitionOrdered generates identical code for those shapes with and
without this change, so those deltas are not attributable to it; most
likely code layout.
Change diff
diff --git a/src/cmd/compile/internal/ssa/_gen/ARM64latelower.rules b/src/cmd/compile/internal/ssa/_gen/ARM64latelower.rules
index d7d2eeb..644d0408 100644
--- a/src/cmd/compile/internal/ssa/_gen/ARM64latelower.rules
+++ b/src/cmd/compile/internal/ssa/_gen/ARM64latelower.rules
@@ -34,43 +34,43 @@
(MOVWUreg x) && ssa.ZeroUpper32Bits(x) => x
// don't extend after proper load
-(MOVBreg x:(MOVBload _ _)) => (MOVDreg x)
-(MOVBUreg x:(MOVBUload _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVBload _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVBUload _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVHload _ _)) => (MOVDreg x)
-(MOVHUreg x:(MOVBUload _ _)) => (MOVDreg x)
-(MOVHUreg x:(MOVHUload _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVBload _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVBUload _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVHload _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVHUload _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVWload _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVBUload _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVHUload _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVWUload _ _)) => (MOVDreg x)
-(MOVBreg x:(MOVBloadidx _ _ _)) => (MOVDreg x)
-(MOVBUreg x:(MOVBUloadidx _ _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVBloadidx _ _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVBUloadidx _ _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVHloadidx _ _ _)) => (MOVDreg x)
-(MOVHUreg x:(MOVBUloadidx _ _ _)) => (MOVDreg x)
-(MOVHUreg x:(MOVHUloadidx _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVBloadidx _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVBUloadidx _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVHloadidx _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVHUloadidx _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVWloadidx _ _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVBUloadidx _ _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVHUloadidx _ _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVWUloadidx _ _ _)) => (MOVDreg x)
-(MOVHreg x:(MOVHloadidx2 _ _ _)) => (MOVDreg x)
-(MOVHUreg x:(MOVHUloadidx2 _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVHloadidx2 _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVHUloadidx2 _ _ _)) => (MOVDreg x)
-(MOVWreg x:(MOVWloadidx4 _ _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVHUloadidx2 _ _ _)) => (MOVDreg x)
-(MOVWUreg x:(MOVWUloadidx4 _ _ _)) => (MOVDreg x)
+(MOVBreg x:(MOVBload _ _)) => x
+(MOVBUreg x:(MOVBUload _ _)) => x
+(MOVHreg x:(MOVBload _ _)) => x
+(MOVHreg x:(MOVBUload _ _)) => x
+(MOVHreg x:(MOVHload _ _)) => x
+(MOVHUreg x:(MOVBUload _ _)) => x
+(MOVHUreg x:(MOVHUload _ _)) => x
+(MOVWreg x:(MOVBload _ _)) => x
+(MOVWreg x:(MOVBUload _ _)) => x
+(MOVWreg x:(MOVHload _ _)) => x
+(MOVWreg x:(MOVHUload _ _)) => x
+(MOVWreg x:(MOVWload _ _)) => x
+(MOVWUreg x:(MOVBUload _ _)) => x
+(MOVWUreg x:(MOVHUload _ _)) => x
+(MOVWUreg x:(MOVWUload _ _)) => x
+(MOVBreg x:(MOVBloadidx _ _ _)) => x
+(MOVBUreg x:(MOVBUloadidx _ _ _)) => x
+(MOVHreg x:(MOVBloadidx _ _ _)) => x
+(MOVHreg x:(MOVBUloadidx _ _ _)) => x
+(MOVHreg x:(MOVHloadidx _ _ _)) => x
+(MOVHUreg x:(MOVBUloadidx _ _ _)) => x
+(MOVHUreg x:(MOVHUloadidx _ _ _)) => x
+(MOVWreg x:(MOVBloadidx _ _ _)) => x
+(MOVWreg x:(MOVBUloadidx _ _ _)) => x
+(MOVWreg x:(MOVHloadidx _ _ _)) => x
+(MOVWreg x:(MOVHUloadidx _ _ _)) => x
+(MOVWreg x:(MOVWloadidx _ _ _)) => x
+(MOVWUreg x:(MOVBUloadidx _ _ _)) => x
+(MOVWUreg x:(MOVHUloadidx _ _ _)) => x
+(MOVWUreg x:(MOVWUloadidx _ _ _)) => x
+(MOVHreg x:(MOVHloadidx2 _ _ _)) => x
+(MOVHUreg x:(MOVHUloadidx2 _ _ _)) => x
+(MOVWreg x:(MOVHloadidx2 _ _ _)) => x
+(MOVWreg x:(MOVHUloadidx2 _ _ _)) => x
+(MOVWreg x:(MOVWloadidx4 _ _ _)) => x
+(MOVWUreg x:(MOVHUloadidx2 _ _ _)) => x
+(MOVWUreg x:(MOVWUloadidx4 _ _ _)) => x
// fold double extensions
(MOVBreg x:(MOVBreg _)) => (MOVDreg x)
diff --git a/src/cmd/compile/internal/ssarewrite/rewritearm64latelower/rewriteARM64latelower.go b/src/cmd/compile/internal/ssarewrite/rewritearm64latelower/rewriteARM64latelower.go
index 4d53475..fef8ef4 100644
--- a/src/cmd/compile/internal/ssarewrite/rewritearm64latelower/rewriteARM64latelower.go
+++ b/src/cmd/compile/internal/ssarewrite/rewritearm64latelower/rewriteARM64latelower.go
@@ -355,25 +355,23 @@
return true
}
// match: (MOVBUreg x:(MOVBUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVBUreg x:(MOVBUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVBUreg x:(MOVBUreg _))
@@ -392,25 +390,23 @@
func rewriteValue_OpARM64MOVBreg(v *ssa.Value) bool {
v_0 := v.Args[0]
// match: (MOVBreg x:(MOVBload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVBreg x:(MOVBloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVBreg x:(MOVBreg _))
@@ -483,58 +479,53 @@
return true
}
// match: (MOVHUreg x:(MOVBUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHUreg x:(MOVHUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHUreg x:(MOVBUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHUreg x:(MOVHUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHUreg x:(MOVHUloadidx2 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUloadidx2 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHUreg x:(MOVBUreg _))
@@ -564,80 +555,73 @@
func rewriteValue_OpARM64MOVHreg(v *ssa.Value) bool {
v_0 := v.Args[0]
// match: (MOVHreg x:(MOVBload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVBUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVHload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVBloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVBUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVHloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVHloadidx2 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHloadidx2 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVHreg x:(MOVBreg _))
@@ -689,91 +673,83 @@
return true
}
// match: (MOVWUreg x:(MOVBUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVHUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVWUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVWUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVBUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVHUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVWUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVWUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVHUloadidx2 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUloadidx2 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVWUloadidx4 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVWUloadidx4 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWUreg x:(MOVBUreg _))
@@ -814,146 +790,133 @@
func rewriteValue_OpARM64MOVWreg(v *ssa.Value) bool {
v_0 := v.Args[0]
// match: (MOVWreg x:(MOVBload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVBUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVHload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVHUload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVWload _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVWload {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVBloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVBUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVBUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVHloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVHUloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVWloadidx _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVWloadidx {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVHloadidx2 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHloadidx2 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVHUloadidx2 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVHUloadidx2 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVWloadidx4 _ _ _))
- // result: (MOVDreg x)
+ // result: x
for {
x := v_0
if x.Op != ssaop.OpARM64MOVWloadidx4 {
break
}
- v.Reset(ssaop.OpARM64MOVDreg)
- v.AddArg(x)
+ v.CopyOf(x)
return true
}
// match: (MOVWreg x:(MOVBreg _))
diff --git a/test/codegen/noextend.go b/test/codegen/noextend.go
index e2c1982..7033a44 100644
--- a/test/codegen/noextend.go
+++ b/test/codegen/noextend.go
@@ -922,3 +922,36 @@
// arm64/v8.1:"SWPALW" -"MOVWU R[0-9]+, R[0-9]+"
return uint64(p.Swap(n))
}
+
+// A sub-word value that is both compared and stored has two uses, so the
+// MOVDreg -> MOVDnop demotion in ARM64latelower.rules, which is gated on
+// x.Uses == 1, does not fire. The extension after the load must still be
+// elided rather than turned into a register-to-register move.
+
+func noExtTwoUsesMOVBload(p *int8, pivot int8, d *int8) bool {
+ x := *p
+ *d = x
+ // arm64:-`MOVD\tR[0-9]+, R[0-9]+`
+ return pivot <= x
+}
+
+func noExtTwoUsesMOVHload(p *int16, pivot int16, d *int16) bool {
+ x := *p
+ *d = x
+ // arm64:-`MOVD\tR[0-9]+, R[0-9]+`
+ return pivot <= x
+}
+
+func noExtTwoUsesMOVBUload(p *uint8, pivot uint8, d *uint8) bool {
+ x := *p
+ *d = x
+ // arm64:-`MOVD\tR[0-9]+, R[0-9]+`
+ return pivot <= x
+}
+
+func noExtTwoUsesMOVHUload(p *uint16, pivot uint16, d *uint16) bool {
+ x := *p
+ *d = x
+ // arm64:-`MOVD\tR[0-9]+, R[0-9]+`
+ return pivot <= x
+}
Change information
- M src/cmd/compile/internal/ssa/_gen/ARM64latelower.rules
- M src/cmd/compile/internal/ssarewrite/rewritearm64latelower/rewriteARM64latelower.go
- M test/codegen/noextend.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. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
Please take a look at the description in https://go-review.googlesource.com/c/go/+/568616
I think before we futz with this code some more, we really need to establish invariants on what a spill/restore pair does. I do worry generally about the fact that we re-extend using the type when a value is spilled/restored, and that re-extension might not match the outer extension op. I can't see any obvious way to make a bug using that mismatch, but I'd be much happier with:
```
(MOVWreg x:(MOVWload ...)) && x.Type.IsSigned() => x
```
because that test is exactly what cmd/compile/internal/arm64/ssa.go:loadByType uses to decide how to extend on spill+restore.
Related details
- Frank Kuehnel
- Martin Möhrmann
- 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. |
Frank Kuehnel (Gerrit)
Frank Kuehnel added 1 comment![]( "Open in Gerrit")
Please take a look at the description in https://go-review.googlesource.com/c/go/+/568616
I think before we futz with this code some more, we really need to establish invariants on what a spill/restore pair does. I do worry generally about the fact that we re-extend using the type when a value is spilled/restored, and that re-extension might not match the outer extension op. I can't see any obvious way to make a bug using that mismatch, but I'd be much happier with:
```
(MOVWreg x:(MOVWload ...)) && x.Type.IsSigned() => x
```
because that test is exactly what cmd/compile/internal/arm64/ssa.go:loadByType uses to decide how to extend on spill+restore.
Read 568616. The paragraph that matters is the last one -- spill/restore does not
preserve the high bits, it re-derives them, and you left the question open. I
think it is answerable, and that the answer is close to what you already landed
for the other direction. Below is the invariant I believe we need, what this CL
does and does not change about it, and what I propose for the next patch set.
### The spill/restore contract
On arm64 (cmd/compile/internal/arm64/ssa.go):
- a spill writes storeByType(x.Type), i.e. the low x.Type.Size() bytes of the
register;
- a restore rewrites the *whole* 64-bit register with loadByType(x.Type): the
low Size() bytes from the slot, upper bits re-extended according to
x.Type.IsSigned() (identity at size 8).
So the only upper-bit pattern that survives an arbitrary spill/restore of x is
extend(x.Type). Nothing else about the defining op is preserved.
### The invariant
An extension op E on top of x may be deleted only if both hold:
(a) x's defining op puts E's result bits in the register at the def. This is
what the rules already test: "proper load", ZeroUpperNBits, the
boolean-producing ops.
(b) loadByType(x.Type) performs the same extension E does. Otherwise a
spill/restore anywhere between the def and a use silently rewrites the
upper bits and (a) stops being true at that use.
(b) is the half that is not written down anywhere -- but it is already enforced
for the zero-extension direction. ssacore/rewrite.go:zeroUpperBits opens with
if x.Type.IsSigned() && 8*x.Type.Size() <= 64-bits {
// A spill/restore sign-extends from the type's width (issue 68227).
return false
}which is exactly (b) for MOVBUreg/MOVHUreg/MOVWUreg, from CL 595675 (#68227).
The "don't extend after proper load" block never got the mirror test. That is
the gap you are pointing at, and I would like to close it in this CL.
### The exposure is not introduced by this CL
MOVDreg compiles to a plain 64-bit MOVD (arm64/ssa.go, the
`case ssaop.OpCopy, ssaop.OpARM64MOVDreg` arm) -- it does not re-extend. So
(MOVWreg x:(MOVWload _ _)) => (MOVDreg x)
already asserts that x's register holds the sign-extended value at the point of
the move, i.e. it already depends on (b) holding across every spill/restore
between the load and the extension, which may sit in another block with calls in
between. Deleting the node does not create the hazard.
It does widen the window: after `=> x`, x's live range reaches every consumer
instead of ending at the move, so there are strictly more points at which a
spill could occur. To me that argues for settling (b) now rather than later, not
for keeping the MOVDreg form, which is exposed to the same mismatch and merely
harder to reach.
### Why I expect the guard to be free on arm64
Every MOVxload takes its type from exactly one place, ARM64.rules:321-327:
(Load <t> ptr mem) && t.IsBoolean() => (MOVBUload ptr mem)
(Load <t> ptr mem) && ssa.Is8BitInt(t) && t.IsSigned() => (MOVBload ptr mem)
... etc
The predicate that picks the flavour is the same size+signedness test that
loadByType uses, so op and type agree at origin. Every later rule that rewrites a
load (offset folding, the loadidx/idx2/idx4 forms, symbol merging) does v.reset
and keeps v.Type; arm64 has no `@x.Block (MOVxload <v.Type> ...)` retyping rules
of the kind amd64 has. Between "lower" and "late lower" only "addressing modes"
runs, and it preserves types. The "pair" pass afterwards keeps the flavour
(MOVWload -> LDPSW, MOVWUload -> LDPW) and threads x.Type through the tuple.
So I expect op flavour and x.Type to agree at every one of these match sites,
and the guard to change zero instructions. I would rather measure that than
assert it.
### Proposed next patch set
1. Guard all 37 rules in the form you wrote, spelled out at each rule. The one
thing worth flagging is that the test follows the *defining load*, not the
outer extension op. 7 of the 37 are sign-extensions over an unsigned load,
where the load already zeroes the bits the extension would set:
(MOVWreg x:(MOVWload _ _)) && x.Type.IsSigned() => x
(MOVWreg x:(MOVBUload _ _)) && !x.Type.IsSigned() => x
(MOVWUreg x:(MOVHUload _ _)) && !x.Type.IsSigned() => x
i.e. the condition is "loadByType(x.Type) is the instruction that defined x,
so a restore re-executes the load". (The converse pairing, an unsigned
extension over a signed load, does not appear in these rules -- it would not
be a no-op.)
If 37 copies of the guard reads badly, I suggest putting the load-op-to-signedness
mapping in one helper in arm64latelower_helpers.go, next to isARM64addcon,
so every rule line becomes the same token.
2. Write (a)/(b) above as a comment over that rule section, next to the existing
"they occur in late lower because ..." note. Three things fall out of it for
free and are worth documenting so the next reader does not re-derive them:
the boolean rule is safe because bool is size 1 unsigned, so the restore is
MOVBU; the double-extension folds are safe because MOVxreg results are size
8, so the restore is MOVD; and the rules assume x.Type.Size() equals the
load's width. I considered testing that width too, but a narrower type on a
wider load is already broken before these rules see it -- the spill drops the
bytes above the type, so the value itself is truncated whether or not an
extension sits on top. That is an SSA well-formedness assumption, so it reads
better as a sentence in the comment than as a condition on 37 rules.
3. Census, to answer "can this actually happen": instrument the guard to log
every match where it fails, build std+cmd for arm64, report the count. If it
is zero, the guard is free and the invariant is enforced locally instead of
holding by an accident spread over three files. If it is not zero, that is a
live bug in the current tree with MOVDreg, and I will file it separately.
4. Verify the guarded compiler emits byte-identical code to the unguarded one
across std+cmd, then re-run the benchmark under the same A/A gate as before.
Related details
- Keith Randall
- Martin Möhrmann
- 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 1 comment![]( "Open in Gerrit")
Note to also look at issue 80577. It is for riscv64, but it demonstrates how tricky this is to get right.
Related details
- Frank Kuehnel
- Martin Möhrmann
- 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. |
Frank Kuehnel (Gerrit)
Frank Kuehnel uploaded new patchset![]( "Open in Gerrit")
Related details
- Frank Kuehnel
- Martin Möhrmann
- 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. |
Frank Kuehnel (Gerrit)
Frank Kuehnel uploaded new patchset![]( "Open in Gerrit")
Related details
- Frank Kuehnel
- Martin Möhrmann
- 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. |
Frank Kuehnel (Gerrit)
Frank Kuehnel added 1 comment![]( "Open in Gerrit")
Thanks — 80577 was the right thing to read, and it changed what I think this CL
should be.
What I found across the backends is not really about arm64, so I put it on the
issue: https://github.com/golang/go/issues/80577#issuecomment-5481715626
The part that matters here is why this is tricky, which looks structural rather
than incidental. Deleting an extension `E` over a value `x` needs two things:
**(a)** `x`'s defining op already leaves `E`'s result in the register, and
**(b)** restoring `x` from a spill slot produces that same result. Every rule in
the tree tests (a). Nothing names (b). And of the two:
- **(a) is half-declarable.** `zeroUpperBits` is a declared per-op attribute in
the `_gen` op definitions, validated by the generator — exactly the right
shape. But it only covers zero extension. There is no attribute for "this op
sign-extends from bit N", so arm64 can declare `MULW` as `zeroUpperBits: 32`
while riscv64's sign-extending `MULW` has nowhere to say so — and that is the
one that got elided unsafely.
- **(b) is not declarable at all.** What a restore puts above `t.Size()` lives
inside `loadByType`, or `loadByRegWidth` on amd64, and is never exposed to the
rules.
- **(a)'s durability is a filing convention.** CL 568616 fixed it by moving
these rules to late lower. Nothing enforces that placement, and s390x has no
late lower pass at all, so the one backend that already tests (b) cannot have
it.
The visible result is six different spellings of (b) across the tree, and four
backends with none.
**So I have taken patch set 2 back down.** It added (b) to the 37 arm64 rules
behind a local helper, and it measured well — the check is evaluated 10504 times
over std+cmd and blocks nothing, both compilers emit identical code across
std+cmd, all.bash passes, and loong64's copy of these rules sits at 20087 sites
with none failing. So cost is not the issue. But free is not the same as right:
a local `isARM64loadMatchesType` would be the seventh spelling of a condition
that ought to exist once, and adding it makes the proliferation worse rather
than better. Patch set 3 is patch set 1 again; patch set 2 is still in the
history if you want to see what it looked like.
What this needs first is an architectural direction — one place that says what a
restore does to the bits above `t.Size()`, that `zeroUpperBits` and the backend
rules can both consult, and ideally a way for an op to declare sign extension
the way it can already declare zero extension. That is a call for the people who
own this area rather than something to settle inside an arm64 CL.
Once there is a direction I will shape this CL to it, including dropping the
optimisation from it if the safety condition should land on its own.
Related details
- Martin Möhrmann
- 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 1 comment![]( "Open in Gerrit")
I agree that it would be good to have the various arch backends report whether restores do sign or zero extend, and then we can use that info in the arch-independent parts (like zeroUpperBits).
Adding a `signExtendedUpperBits` field to ops that guarantee sign extension would also be good. (Although there is nowhere that uses such info currently?)
Related details
- Frank Kuehnel
- Martin Möhrmann
- 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. |
Frank Kuehnel (Gerrit)
Frank Kuehnel added 1 comment![]( "Open in Gerrit")
Makes sense. Since that refactoring affects multiple backends, should we scope the architectural changes to a separate issue/proposal so this fix isn't blocked, or would you prefer to agree on the signExtendedUpperBits API design first?
Related details
- Keith Randall
- Martin Möhrmann
- 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 1 comment![]( "Open in Gerrit")
The refactoring/architectural change should be first, hopefully with no semantic changes, just a reorg of existing code. Then we can make this CL on top of that CL.
Related details
- Frank Kuehnel
- Martin Möhrmann
- 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. |
Frank Kuehnel (Gerrit)
Frank Kuehnel added 1 comment![]( "Open in Gerrit")
I opened a new CL for discussion:
(https://go-review.googlesource.com/c/go/+/825904/1/src/cmd/compile/internal/ssa/spill-contract.md)
Related details
- Keith Randall
- Martin Möhrmann
- 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. |