From 6a712dfac19f2117fd54c7af2280c67be07727ac Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Tue, 21 Feb 2017 15:20:38 -0500 Subject: [PATCH] [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 TryBot-Result: Gobot Gobot Reviewed-by: Bill O'Farrell Reviewed-by: Cherry Zhang --- src/cmd/compile/internal/ssa/export_test.go | 5 ++ src/cmd/compile/internal/ssa/gen/S390X.rules | 2 +- src/cmd/compile/internal/ssa/loop_test.go | 87 ++++++++++++++++++++ src/cmd/compile/internal/ssa/rewriteS390X.go | 4 +- 4 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 src/cmd/compile/internal/ssa/loop_test.go 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 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 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) -- 2.48.1