cmd/compile: zero extend when replacing load-hit-store on s390x

Keith pointed out that these rules should zero extend during the review of CL 36845. In practice the generic rules are responsible for eliminating most load-hit-stores and they do not have this problem. When the s390x rules are triggered any cast following the elided load-hit-store is kept because of the sequence the rules are applied in (i.e. the load is removed before the zero extension gets a chance to be merged into the load). It is therefore not clear that this issue results in any functional bugs. This CL includes a test, but it only tests the generic rules currently. Change-Id: Idbc43c782097a3fb159be293ec3138c5b36858ad Reviewed-on: https://go-review.googlesource.com/37154 Run-TryBot: Michael Munday <munday@ca.ibm.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
golang · Feb 22, 2017 · 094992e · 094992e
1 parent 11b2830
commit 094992e
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 9 deletions.
diff --git a/src/cmd/compile/internal/gc/testdata/loadstore.go b/src/cmd/compile/internal/gc/testdata/loadstore.go
@@ -102,12 +102,120 @@ func testDeadStorePanic() {
 	}
 }
 
+//go:noinline
+func loadHitStore8(x int8, p *int8) int32 {
+	x *= x           // try to trash high bits (arch-dependent)
+	*p = x           // store
+	return int32(*p) // load and cast
+}
+
+//go:noinline
+func loadHitStoreU8(x uint8, p *uint8) uint32 {
+	x *= x            // try to trash high bits (arch-dependent)
+	*p = x            // store
+	return uint32(*p) // load and cast
+}
+
+//go:noinline
+func loadHitStore16(x int16, p *int16) int32 {
+	x *= x           // try to trash high bits (arch-dependent)
+	*p = x           // store
+	return int32(*p) // load and cast
+}
+
+//go:noinline
+func loadHitStoreU16(x uint16, p *uint16) uint32 {
+	x *= x            // try to trash high bits (arch-dependent)
+	*p = x            // store
+	return uint32(*p) // load and cast
+}
+
+//go:noinline
+func loadHitStore32(x int32, p *int32) int64 {
+	x *= x           // try to trash high bits (arch-dependent)
+	*p = x           // store
+	return int64(*p) // load and cast
+}
+
+//go:noinline
+func loadHitStoreU32(x uint32, p *uint32) uint64 {
+	x *= x            // try to trash high bits (arch-dependent)
+	*p = x            // store
+	return uint64(*p) // load and cast
+}
+
+func testLoadHitStore() {
+	// Test that sign/zero extensions are kept when a load-hit-store
+	// is replaced by a register-register move.
+	{
+		var in int8 = (1 << 6) + 1
+		var p int8
+		got := loadHitStore8(in, &p)
+		want := int32(in * in)
+		if got != want {
+			fmt.Println("testLoadHitStore (int8) failed. want =", want, ", got =", got)
+			failed = true
+		}
+	}
+	{
+		var in uint8 = (1 << 6) + 1
+		var p uint8
+		got := loadHitStoreU8(in, &p)
+		want := uint32(in * in)
+		if got != want {
+			fmt.Println("testLoadHitStore (uint8) failed. want =", want, ", got =", got)
+			failed = true
+		}
+	}
+	{
+		var in int16 = (1 << 10) + 1
+		var p int16
+		got := loadHitStore16(in, &p)
+		want := int32(in * in)
+		if got != want {
+			fmt.Println("testLoadHitStore (int16) failed. want =", want, ", got =", got)
+			failed = true
+		}
+	}
+	{
+		var in uint16 = (1 << 10) + 1
+		var p uint16
+		got := loadHitStoreU16(in, &p)
+		want := uint32(in * in)
+		if got != want {
+			fmt.Println("testLoadHitStore (uint16) failed. want =", want, ", got =", got)
+			failed = true
+		}
+	}
+	{
+		var in int32 = (1 << 30) + 1
+		var p int32
+		got := loadHitStore32(in, &p)
+		want := int64(in * in)
+		if got != want {
+			fmt.Println("testLoadHitStore (int32) failed. want =", want, ", got =", got)
+			failed = true
+		}
+	}
+	{
+		var in uint32 = (1 << 30) + 1
+		var p uint32
+		got := loadHitStoreU32(in, &p)
+		want := uint64(in * in)
+		if got != want {
+			fmt.Println("testLoadHitStore (uint32) failed. want =", want, ", got =", got)
+			failed = true
+		}
+	}
+}
+
 func main() {
 
 	testLoadStoreOrder()
 	testStoreSize()
 	testExtStore()
 	testDeadStorePanic()
+	testLoadHitStore()
 
 	if failed {
 		panic("failed")

diff --git a/src/cmd/compile/internal/ssa/gen/S390X.rules b/src/cmd/compile/internal/ssa/gen/S390X.rules
@@ -703,9 +703,9 @@
 (MOVWZreg x:(MOVWZloadidx [off] {sym} ptr idx mem)) && x.Uses == 1 && clobber(x) -> @x.Block (MOVWZloadidx <v.Type> [off] {sym} ptr idx mem)
 
 // replace load from same location as preceding store with copy
-(MOVBZload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVDreg x)
-(MOVHZload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVDreg x)
-(MOVWZload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVDreg x)
+(MOVBZload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVBZreg x)
+(MOVHZload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVHZreg x)
+(MOVWZload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVWZreg x)
 (MOVDload [off] {sym} ptr (MOVDstore [off2] {sym2} ptr2 x _)) && sym == sym2 && off == off2 && isSamePtr(ptr, ptr2) -> (MOVDreg x)
 
 // Don't extend before storing

diff --git a/src/cmd/compile/internal/ssa/rewriteS390X.go b/src/cmd/compile/internal/ssa/rewriteS390X.go
@@ -7904,7 +7904,7 @@ func rewriteValueS390X_OpS390XMOVBZload(v *Value, config *Config) bool {
 	_ = b
 	// match: (MOVBZload [off] {sym} ptr (MOVBstore [off2] {sym2} ptr2 x _))
 	// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
-	// result: (MOVDreg x)
+	// result: (MOVBZreg x)
 	for {
 		off := v.AuxInt
 		sym := v.Aux
@@ -7920,7 +7920,7 @@ func rewriteValueS390X_OpS390XMOVBZload(v *Value, config *Config) bool {
 		if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
 			break
 		}
-		v.reset(OpS390XMOVDreg)
+		v.reset(OpS390XMOVBZreg)
 		v.AddArg(x)
 		return true
 	}
@@ -11662,7 +11662,7 @@ func rewriteValueS390X_OpS390XMOVHZload(v *Value, config *Config) bool {
 	_ = b
 	// match: (MOVHZload [off] {sym} ptr (MOVHstore [off2] {sym2} ptr2 x _))
 	// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
-	// result: (MOVDreg x)
+	// result: (MOVHZreg x)
 	for {
 		off := v.AuxInt
 		sym := v.Aux
@@ -11678,7 +11678,7 @@ func rewriteValueS390X_OpS390XMOVHZload(v *Value, config *Config) bool {
 		if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
 			break
 		}
-		v.reset(OpS390XMOVDreg)
+		v.reset(OpS390XMOVHZreg)
 		v.AddArg(x)
 		return true
 	}
@@ -13047,7 +13047,7 @@ func rewriteValueS390X_OpS390XMOVWZload(v *Value, config *Config) bool {
 	_ = b
 	// match: (MOVWZload [off] {sym} ptr (MOVWstore [off2] {sym2} ptr2 x _))
 	// cond: sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)
-	// result: (MOVDreg x)
+	// result: (MOVWZreg x)
 	for {
 		off := v.AuxInt
 		sym := v.Aux
@@ -13063,7 +13063,7 @@ func rewriteValueS390X_OpS390XMOVWZload(v *Value, config *Config) bool {
 		if !(sym == sym2 && off == off2 && isSamePtr(ptr, ptr2)) {
 			break
 		}
-		v.reset(OpS390XMOVDreg)
+		v.reset(OpS390XMOVWZreg)
 		v.AddArg(x)
 		return true
 	}