[release-branch.go1.8] cmd/compile: fix merging of s390x conditional moves into branch conditions

A type conversion inserted between MOVD{LT,LE,GT,GE,EQ,NE} and CMPWconst
by CL 36256 broke the rewrite rule designed to merge the two.
This results in simple for loops (e.g. for i := 0; i < N; i++ {})
emitting two comparisons instead of one, plus a conditional move.

This CL explicitly types the input to CMPWconst so that the type conversion
can be omitted. It also adds a test to check that conditional moves aren't
emitted for loops with 'less than' conditions (i.e. i < N) on s390x.

Fixes #19227.

Change-Id: I44958eebf6c74c5819b2a9511caf3c47c20fbf45
Reviewed-on: https://go-review.googlesource.com/37536
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bill O'Farrell <billotosyr@gmail.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>

4 files changed, 95 insertions, 3 deletions

diff --git a/src/cmd/compile/internal/ssa/export_test.go b/src/cmd/compile/internal/ssa/export_test.go
index 3a9357dfae..ee6ed51d73 100644
--- a/src/cmd/compile/internal/ssa/export_test.go
+++ b/src/cmd/compile/internal/ssa/export_test.go
@@ -6,6 +6,7 @@ package ssa
 
 import (
 	"cmd/internal/obj"
+	"cmd/internal/obj/s390x"
 	"cmd/internal/obj/x86"
 	"testing"
 )
@@ -21,6 +22,10 @@ func testConfig(t testing.TB) *Config {
 	return NewConfig("amd64", DummyFrontend{t}, testCtxt, true)
 }
 
+func testConfigS390X(t testing.TB) *Config {
+	return NewConfig("s390x", DummyFrontend{t}, obj.Linknew(&s390x.Links390x), true)
+}
+
 // DummyFrontend is a test-only frontend.
 // It assumes 64 bit integers and pointers.
 type DummyFrontend struct {
diff --git a/src/cmd/compile/internal/ssa/gen/S390X.rules b/src/cmd/compile/internal/ssa/gen/S390X.rules
index 4ba4ddb11c..7ecea02dae 100644
--- a/src/cmd/compile/internal/ssa/gen/S390X.rules
+++ b/src/cmd/compile/internal/ssa/gen/S390X.rules
@@ -440,7 +440,7 @@
 (If (MOVDGTnoinv (MOVDconst [0]) (MOVDconst [1]) cmp) yes no) -> (GTF cmp yes no)
 (If (MOVDGEnoinv (MOVDconst [0]) (MOVDconst [1]) cmp) yes no) -> (GEF cmp yes no)
 
-(If cond yes no) -> (NE (CMPWconst [0] (MOVBZreg cond)) yes no)
+(If cond yes no) -> (NE (CMPWconst [0] (MOVBZreg <config.fe.TypeBool()> cond)) yes no)
 
 // ***************************
 // Above: lowering rules
diff --git a/src/cmd/compile/internal/ssa/loop_test.go b/src/cmd/compile/internal/ssa/loop_test.go
new file mode 100644
index 0000000000..69a49627a1
--- /dev/null
+++ b/src/cmd/compile/internal/ssa/loop_test.go
@@ -0,0 +1,87 @@
+// Copyright 2017 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 ssa
+
+import (
+	"testing"
+)
+
+func TestLoopConditionS390X(t *testing.T) {
+	// Test that a simple loop condition does not generate a conditional
+	// move (issue #19227).
+	//
+	// MOVDLT is generated when Less64 is lowered but should be
+	// optimized into an LT branch.
+	//
+	// For example, compiling the following loop:
+	//
+	//   for i := 0; i < N; i++ {
+	//     sum += 3
+	//   }
+	//
+	// should generate assembly similar to:
+	//   loop:
+	//     CMP    R0, R1
+	//     BGE    done
+	//     ADD    $3, R4
+	//     ADD    $1, R1
+	//     BR     loop
+	//   done:
+	//
+	// rather than:
+	// loop:
+	//     MOVD   $0, R2
+	//     MOVD   $1, R3
+	//     CMP    R0, R1
+	//     MOVDLT R2, R3
+	//     CMPW   R2, $0
+	//     BNE    done
+	//     ADD    $3, R4
+	//     ADD    $1, R1
+	//     BR     loop
+	//   done:
+	//
+	c := testConfigS390X(t)
+	fun := Fun(c, "entry",
+		Bloc("entry",
+			Valu("mem", OpInitMem, TypeMem, 0, nil),
+			Valu("SP", OpSP, TypeUInt64, 0, nil),
+			Valu("Nptr", OpOffPtr, TypeInt64Ptr, 8, nil, "SP"),
+			Valu("ret", OpOffPtr, TypeInt64Ptr, 16, nil, "SP"),
+			Valu("N", OpLoad, TypeInt64, 0, nil, "Nptr", "mem"),
+			Valu("starti", OpConst64, TypeInt64, 0, nil),
+			Valu("startsum", OpConst64, TypeInt64, 0, nil),
+			Goto("b1")),
+		Bloc("b1",
+			Valu("phii", OpPhi, TypeInt64, 0, nil, "starti", "i"),
+			Valu("phisum", OpPhi, TypeInt64, 0, nil, "startsum", "sum"),
+			Valu("cmp1", OpLess64, TypeBool, 0, nil, "phii", "N"),
+			If("cmp1", "b2", "b3")),
+		Bloc("b2",
+			Valu("c1", OpConst64, TypeInt64, 1, nil),
+			Valu("i", OpAdd64, TypeInt64, 0, nil, "phii", "c1"),
+			Valu("c3", OpConst64, TypeInt64, 3, nil),
+			Valu("sum", OpAdd64, TypeInt64, 0, nil, "phisum", "c3"),
+			Goto("b1")),
+		Bloc("b3",
+			Valu("store", OpStore, TypeMem, 8, nil, "ret", "phisum", "mem"),
+			Exit("store")))
+	CheckFunc(fun.f)
+	Compile(fun.f)
+	CheckFunc(fun.f)
+
+	checkOpcodeCounts(t, fun.f, map[Op]int{
+		OpS390XMOVDLT:    0,
+		OpS390XMOVDGT:    0,
+		OpS390XMOVDLE:    0,
+		OpS390XMOVDGE:    0,
+		OpS390XMOVDEQ:    0,
+		OpS390XMOVDNE:    0,
+		OpS390XCMP:       1,
+		OpS390XCMPWconst: 0,
+	})
+
+	fun.f.Free()
+}
diff --git a/src/cmd/compile/internal/ssa/rewriteS390X.go b/src/cmd/compile/internal/ssa/rewriteS390X.go
index ae86541a03..866cf50041 100644
--- a/src/cmd/compile/internal/ssa/rewriteS390X.go
+++ b/src/cmd/compile/internal/ssa/rewriteS390X.go
@@ -18242,7 +18242,7 @@ func rewriteBlockS390X(b *Block, config *Config) bool {
 		}
 		// match: (If cond yes no)
 		// cond:
-		// result: (NE (CMPWconst [0] (MOVBZreg cond)) yes no)
+		// result: (NE (CMPWconst [0] (MOVBZreg <config.fe.TypeBool()> cond)) yes no)
 		for {
 			v := b.Control
 			_ = v
@@ -18252,7 +18252,7 @@ func rewriteBlockS390X(b *Block, config *Config) bool {
 			b.Kind = BlockS390XNE
 			v0 := b.NewValue0(v.Line, OpS390XCMPWconst, TypeFlags)
 			v0.AuxInt = 0
-			v1 := b.NewValue0(v.Line, OpS390XMOVBZreg, config.fe.TypeUInt64())
+			v1 := b.NewValue0(v.Line, OpS390XMOVBZreg, config.fe.TypeBool())
 			v1.AddArg(cond)
 			v0.AddArg(v1)
 			b.SetControl(v0)