Skip to content

Commit 82cfda2

Browse files
cherrymuirsc
cherrymui
authored and
rsc
committed
[release-branch.go1.8] cmd/compile: fix subword store/load elision for MIPS
Apply the fix in CL 44355 to MIPS. ARM64 has these rules but commented out for performance reason. Fix the commented rules, in case they are enabled in the future. Enhance the test so it triggers the failure on ARM and MIPS without the fix. Updates #20530. Change-Id: I82d77448e3939a545fe519d0a29a164f8fa5417c Reviewed-on: https://go-review.googlesource.com/44430 Run-TryBot: Cherry Zhang <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: David Chase <[email protected]> Reviewed-on: https://go-review.googlesource.com/70840 Run-TryBot: Russ Cox <[email protected]> Reviewed-by: Cherry Zhang <[email protected]>
1 parent 9ccc292 commit 82cfda2

File tree

4 files changed

+45
-33
lines changed

4 files changed

+45
-33
lines changed

‎src/cmd/compile/internal/ssa/gen/ARM64.rules

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -679,14 +679,14 @@
679679
(MOVWstore [off] {sym} ptr (MOVDconst [0]) mem) -> (MOVWstorezero [off] {sym} ptr mem)
680680
(MOVDstore [off] {sym} ptr (MOVDconst [0]) mem) -> (MOVDstorezero [off] {sym} ptr mem)
681681

682-
// replace load from same location as preceding store with copy
682+
// replace load from same location as preceding store with zero/sign extension (or copy in case of full width)
683683
// these seem to have bad interaction with other rules, resulting in slower code
684-
//(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
685-
//(MOVBUload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
686-
//(MOVHload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
687-
//(MOVHUload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
688-
//(MOVWload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
689-
//(MOVWUload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
684+
//(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVBreg x)
685+
//(MOVBUload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVBUreg x)
686+
//(MOVHload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVHreg x)
687+
//(MOVHUload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVHUreg x)
688+
//(MOVWload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVWreg x)
689+
//(MOVWUload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVWUreg x)
690690
//(MOVDload [off] {sym} ptr (MOVDstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
691691
//(FMOVSload [off] {sym} ptr (FMOVSstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
692692
//(FMOVDload [off] {sym} ptr (FMOVDstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x

‎src/cmd/compile/internal/ssa/gen/MIPS.rules

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -522,11 +522,11 @@
522522
(MOVWstorezero [off1] {sym1} (MOVWaddr [off2] {sym2} ptr) mem) && canMergeSym(sym1,sym2) ->
523523
(MOVWstorezero [off1+off2] {mergeSym(sym1,sym2)} ptr mem)
524524

525-
// replace load from same location as preceding store with copy
526-
(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type) -> x
527-
(MOVBUload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type) -> x
528-
(MOVHload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type) -> x
529-
(MOVHUload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type) -> x
525+
// replace load from same location as preceding store with zero/sign extension (or copy in case of full width)
526+
(MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVBreg x)
527+
(MOVBUload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVBUreg x)
528+
(MOVHload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVHreg x)
529+
(MOVHUload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVHUreg x)
530530
(MOVWload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
531531
(MOVFload [off] {sym} ptr (MOVFstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x
532532
(MOVDload [off] {sym} ptr (MOVDstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> x

‎src/cmd/compile/internal/ssa/rewriteMIPS.go

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3332,8 +3332,8 @@ func rewriteValueMIPS_OpMIPSMOVBUload(v *Value, config *Config) bool {
33323332
return true
33333333
}
33343334
// match: (MOVBUload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _))
3335-
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type)
3336-
// result: x
3335+
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
3336+
// result: (MOVBUreg x)
33373337
for {
33383338
off := v.AuxInt
33393339
sym := v.Aux
@@ -3346,11 +3346,10 @@ func rewriteValueMIPS_OpMIPSMOVBUload(v *Value, config *Config) bool {
33463346
sym2 := v_1.Aux
33473347
ptr2 := v_1.Args[0]
33483348
x := v_1.Args[1]
3349-
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type)) {
3349+
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
33503350
break
33513351
}
3352-
v.reset(OpCopy)
3353-
v.Type = x.Type
3352+
v.reset(OpMIPSMOVBUreg)
33543353
v.AddArg(x)
33553354
return true
33563355
}
@@ -3490,8 +3489,8 @@ func rewriteValueMIPS_OpMIPSMOVBload(v *Value, config *Config) bool {
34903489
return true
34913490
}
34923491
// match: (MOVBload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _))
3493-
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type)
3494-
// result: x
3492+
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
3493+
// result: (MOVBreg x)
34953494
for {
34963495
off := v.AuxInt
34973496
sym := v.Aux
@@ -3504,11 +3503,10 @@ func rewriteValueMIPS_OpMIPSMOVBload(v *Value, config *Config) bool {
35043503
sym2 := v_1.Aux
35053504
ptr2 := v_1.Args[0]
35063505
x := v_1.Args[1]
3507-
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type)) {
3506+
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
35083507
break
35093508
}
3510-
v.reset(OpCopy)
3511-
v.Type = x.Type
3509+
v.reset(OpMIPSMOVBreg)
35123510
v.AddArg(x)
35133511
return true
35143512
}
@@ -4148,8 +4146,8 @@ func rewriteValueMIPS_OpMIPSMOVHUload(v *Value, config *Config) bool {
41484146
return true
41494147
}
41504148
// match: (MOVHUload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _))
4151-
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type)
4152-
// result: x
4149+
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
4150+
// result: (MOVHUreg x)
41534151
for {
41544152
off := v.AuxInt
41554153
sym := v.Aux
@@ -4162,11 +4160,10 @@ func rewriteValueMIPS_OpMIPSMOVHUload(v *Value, config *Config) bool {
41624160
sym2 := v_1.Aux
41634161
ptr2 := v_1.Args[0]
41644162
x := v_1.Args[1]
4165-
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && !isSigned(x.Type)) {
4163+
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
41664164
break
41674165
}
4168-
v.reset(OpCopy)
4169-
v.Type = x.Type
4166+
v.reset(OpMIPSMOVHUreg)
41704167
v.AddArg(x)
41714168
return true
41724169
}
@@ -4330,8 +4327,8 @@ func rewriteValueMIPS_OpMIPSMOVHload(v *Value, config *Config) bool {
43304327
return true
43314328
}
43324329
// match: (MOVHload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _))
4333-
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type)
4334-
// result: x
4330+
// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
4331+
// result: (MOVHreg x)
43354332
for {
43364333
off := v.AuxInt
43374334
sym := v.Aux
@@ -4344,11 +4341,10 @@ func rewriteValueMIPS_OpMIPSMOVHload(v *Value, config *Config) bool {
43444341
sym2 := v_1.Aux
43454342
ptr2 := v_1.Args[0]
43464343
x := v_1.Args[1]
4347-
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) && isSigned(x.Type)) {
4344+
if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
43484345
break
43494346
}
4350-
v.reset(OpCopy)
4351-
v.Type = x.Type
4347+
v.reset(OpMIPSMOVHreg)
43524348
v.AddArg(x)
43534349
return true
43544350
}

‎test/fixedbugs/issue20530.go

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,27 @@ package main
88

99
var a uint8
1010

11-
func main() {
11+
//go:noinline
12+
func f() {
1213
b := int8(func() int32 { return -1 }())
1314
a = uint8(b)
1415
if int32(a) != 255 {
1516
// Failing case prints 'got 255 expected 255'
1617
println("got", a, "expected 255")
1718
}
1819
}
20+
21+
//go:noinline
22+
func g() {
23+
b := int8(func() uint32 { return 0xffffffff }())
24+
a = uint8(b)
25+
if int32(a) != 255 {
26+
// Failing case prints 'got 255 expected 255'
27+
println("got", a, "expected 255")
28+
}
29+
}
30+
31+
func main() {
32+
f()
33+
g()
34+
}

0 commit comments

Comments
 (0)