From 334d410ae35a28d770fe43009ca346b4387c1399 Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Sun, 12 Apr 2020 10:45:24 +0100 Subject: [PATCH] cmd/compile: fix incorrect block for s390x Select1 op When inserting Select0 and Select1 ops we need to ensure that they live in the same block as their argument. This is because they need to be scheduled immediately after their argument for register and flag allocation to work correctly. Fixes #38356. Change-Id: Iba384dbe87010f1c7c4ce909f08011e5f1de7fd5 Reviewed-on: https://go-review.googlesource.com/c/go/+/227879 Run-TryBot: Michael Munday TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall --- src/cmd/compile/internal/ssa/gen/S390X.rules | 15 ++++-- src/cmd/compile/internal/ssa/rewriteS390X.go | 14 +++-- test/fixedbugs/issue38356.go | 54 ++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 test/fixedbugs/issue38356.go diff --git a/src/cmd/compile/internal/ssa/gen/S390X.rules b/src/cmd/compile/internal/ssa/gen/S390X.rules index c88919a72a..72dbb5f87b 100644 --- a/src/cmd/compile/internal/ssa/gen/S390X.rules +++ b/src/cmd/compile/internal/ssa/gen/S390X.rules @@ -1218,10 +1218,17 @@ (FCMP (FMOVDconst [c]) x) && auxTo64F(c) == 0 -> (InvertFlags (LTDBR x)) (FCMPS (FMOVSconst [c]) x) && auxTo32F(c) == 0 -> (InvertFlags (LTEBR x)) -// FSUB, FSUBS, FADD, FADDS now produce a flag, so when a comparison against zero instruction (e.g: LTDBR) is following -// one of those instructions, we can use the generated flag and remove the comparison instruction. -(LTDBR (Select0 x:(F(ADD|SUB) _ _))) -> (Select1 x) -(LTEBR (Select0 x:(F(ADDS|SUBS) _ _))) -> (Select1 x) +// FSUB, FSUBS, FADD, FADDS now produce a condition code representing the +// comparison of the result with 0.0. If a compare with zero instruction +// (e.g. LTDBR) is following one of those instructions, we can use the +// generated flag and remove the comparison instruction. +// Note: when inserting Select1 ops we need to ensure they are in the +// same block as their argument. We could also use @x.Block for this +// but moving the flag generating value to a different block seems to +// increase the likelihood that the flags value will have to be regenerated +// by flagalloc which is not what we want. +(LTDBR (Select0 x:(F(ADD|SUB) _ _))) && b == x.Block -> (Select1 x) +(LTEBR (Select0 x:(F(ADDS|SUBS) _ _))) && b == x.Block -> (Select1 x) // Fold memory operations into operations. // Exclude global data (SB) because these instructions cannot handle relative addresses. diff --git a/src/cmd/compile/internal/ssa/rewriteS390X.go b/src/cmd/compile/internal/ssa/rewriteS390X.go index 43fe3d8756..fda0bc6b34 100644 --- a/src/cmd/compile/internal/ssa/rewriteS390X.go +++ b/src/cmd/compile/internal/ssa/rewriteS390X.go @@ -8196,14 +8196,16 @@ func rewriteValueS390X_OpS390XLOCGR(v *Value) bool { } func rewriteValueS390X_OpS390XLTDBR(v *Value) bool { v_0 := v.Args[0] + b := v.Block // match: (LTDBR (Select0 x:(FADD _ _))) + // cond: b == x.Block // result: (Select1 x) for { if v_0.Op != OpSelect0 { break } x := v_0.Args[0] - if x.Op != OpS390XFADD { + if x.Op != OpS390XFADD || !(b == x.Block) { break } v.reset(OpSelect1) @@ -8211,13 +8213,14 @@ func rewriteValueS390X_OpS390XLTDBR(v *Value) bool { return true } // match: (LTDBR (Select0 x:(FSUB _ _))) + // cond: b == x.Block // result: (Select1 x) for { if v_0.Op != OpSelect0 { break } x := v_0.Args[0] - if x.Op != OpS390XFSUB { + if x.Op != OpS390XFSUB || !(b == x.Block) { break } v.reset(OpSelect1) @@ -8228,14 +8231,16 @@ func rewriteValueS390X_OpS390XLTDBR(v *Value) bool { } func rewriteValueS390X_OpS390XLTEBR(v *Value) bool { v_0 := v.Args[0] + b := v.Block // match: (LTEBR (Select0 x:(FADDS _ _))) + // cond: b == x.Block // result: (Select1 x) for { if v_0.Op != OpSelect0 { break } x := v_0.Args[0] - if x.Op != OpS390XFADDS { + if x.Op != OpS390XFADDS || !(b == x.Block) { break } v.reset(OpSelect1) @@ -8243,13 +8248,14 @@ func rewriteValueS390X_OpS390XLTEBR(v *Value) bool { return true } // match: (LTEBR (Select0 x:(FSUBS _ _))) + // cond: b == x.Block // result: (Select1 x) for { if v_0.Op != OpSelect0 { break } x := v_0.Args[0] - if x.Op != OpS390XFSUBS { + if x.Op != OpS390XFSUBS || !(b == x.Block) { break } v.reset(OpSelect1) diff --git a/test/fixedbugs/issue38356.go b/test/fixedbugs/issue38356.go new file mode 100644 index 0000000000..a1c7f4675a --- /dev/null +++ b/test/fixedbugs/issue38356.go @@ -0,0 +1,54 @@ +// compile + +// Copyright 2020 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. + +// Make sure floating point operations that generate flags +// are scheduled correctly on s390x. + +package p + +func f1(x, y float64, z int) float64 { + a := x + y // generate flags + if z == 0 { // create basic block that does not clobber flags + return a + } + if a > 0 { // use flags in different basic block + return y + } + return x +} + +func f2(x, y float64, z int) float64 { + a := x - y // generate flags + if z == 0 { // create basic block that does not clobber flags + return a + } + if a > 0 { // use flags in different basic block + return y + } + return x +} + +func f3(x, y float32, z int) float32 { + a := x + y // generate flags + if z == 0 { // create basic block that does not clobber flags + return a + } + if a > 0 { // use flags in different basic block + return y + } + return x +} + +func f4(x, y float32, z int) float32 { + a := x - y // generate flags + if z == 0 { // create basic block that does not clobber flags + return a + } + if a > 0 { // use flags in different basic block + return y + } + return x +}