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

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>

diff --git a/src/cmd/compile/internal/ssa/export_test.go b/src/cmd/compile/internal/ssa/export_test.go
index 3a9357dfae4f10d47f15f2917d93fca6bdb9240d..ee6ed51d730ebce5d4f8957704c58a8c5be1cbf3 100644 (file)
--- 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 4ba4ddb11c455a53d34bf472f4830728489ff34c..7ecea02daeddc5a462a66e0889f31f81384f4dba 100644 (file)
--- 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 (file)
index 0000000..69a4962
--- /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 ae86541a03d224e3c6e1ec71c09fb9228cfd71aa..866cf50041540908f1574aa3f8c08649230c6f0d 100644 (file)
--- 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)