mirror of
https://github.com/golang/go
synced 2024-10-02 22:25:08 +00:00
cmd/compile: fix ordering for short-circuiting ops
Make sure the side effects inside short-circuited operations (&& and ||) happen correctly. Before this CL, we attached the side effects to the node itself using exprInPlace. That caused other side effects in sibling expressions to get reordered with respect to the short circuit side effect. Instead, rewrite a && b like: r := a if r { r = b } That code we can keep correctly ordered with respect to other side-effects extracted from part of a big expression. exprInPlace seems generally unsafe. But this was the only case where exprInPlace is called not at the top level of an expression, so I don't think the other uses can actually trigger an issue (there can't be a sibling expression). TODO: maybe those cases don't need "in place", and we can retire that function generally. This CL needed a small tweak to the SSA generation of OIF so that the short circuit optimization still triggers. The short circuit optimization looks for triangle but not diamonds, so don't bother allocating a block if it will be empty. Go 1 benchmarks are in the noise. Fixes #30566 Change-Id: I19c04296bea63cbd6ad05f87a63b005029123610 Reviewed-on: https://go-review.googlesource.com/c/go/+/165617 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>
This commit is contained in:
parent
7778b5ab5a
commit
4a9064ef41
|
@ -1130,14 +1130,40 @@ func (o *Order) expr(n, lhs *Node) *Node {
|
|||
}
|
||||
|
||||
case OANDAND, OOROR:
|
||||
mark := o.markTemp()
|
||||
n.Left = o.expr(n.Left, nil)
|
||||
// ... = LHS && RHS
|
||||
//
|
||||
// var r bool
|
||||
// r = LHS
|
||||
// if r { // or !r, for OROR
|
||||
// r = RHS
|
||||
// }
|
||||
// ... = r
|
||||
|
||||
// Clean temporaries from first branch at beginning of second.
|
||||
// Leave them on the stack so that they can be killed in the outer
|
||||
// context in case the short circuit is taken.
|
||||
n.Right = addinit(n.Right, o.cleanTempNoPop(mark))
|
||||
n.Right = o.exprInPlace(n.Right)
|
||||
r := o.newTemp(n.Type, false)
|
||||
|
||||
// Evaluate left-hand side.
|
||||
lhs := o.expr(n.Left, nil)
|
||||
o.out = append(o.out, typecheck(nod(OAS, r, lhs), ctxStmt))
|
||||
|
||||
// Evaluate right-hand side, save generated code.
|
||||
saveout := o.out
|
||||
o.out = nil
|
||||
t := o.markTemp()
|
||||
rhs := o.expr(n.Right, nil)
|
||||
o.out = append(o.out, typecheck(nod(OAS, r, rhs), ctxStmt))
|
||||
o.cleanTemp(t)
|
||||
gen := o.out
|
||||
o.out = saveout
|
||||
|
||||
// If left-hand side doesn't cause a short-circuit, issue right-hand side.
|
||||
nif := nod(OIF, r, nil)
|
||||
if n.Op == OANDAND {
|
||||
nif.Nbody.Set(gen)
|
||||
} else {
|
||||
nif.Rlist.Set(gen)
|
||||
}
|
||||
o.out = append(o.out, nif)
|
||||
n = r
|
||||
|
||||
case OCALLFUNC,
|
||||
OCALLINTER,
|
||||
|
|
|
@ -994,26 +994,32 @@ func (s *state) stmt(n *Node) {
|
|||
s.assign(n.Left, r, deref, skip)
|
||||
|
||||
case OIF:
|
||||
bThen := s.f.NewBlock(ssa.BlockPlain)
|
||||
bEnd := s.f.NewBlock(ssa.BlockPlain)
|
||||
var bElse *ssa.Block
|
||||
var likely int8
|
||||
if n.Likely() {
|
||||
likely = 1
|
||||
}
|
||||
var bThen *ssa.Block
|
||||
if n.Nbody.Len() != 0 {
|
||||
bThen = s.f.NewBlock(ssa.BlockPlain)
|
||||
} else {
|
||||
bThen = bEnd
|
||||
}
|
||||
var bElse *ssa.Block
|
||||
if n.Rlist.Len() != 0 {
|
||||
bElse = s.f.NewBlock(ssa.BlockPlain)
|
||||
s.condBranch(n.Left, bThen, bElse, likely)
|
||||
} else {
|
||||
s.condBranch(n.Left, bThen, bEnd, likely)
|
||||
bElse = bEnd
|
||||
}
|
||||
s.condBranch(n.Left, bThen, bElse, likely)
|
||||
|
||||
s.startBlock(bThen)
|
||||
s.stmtList(n.Nbody)
|
||||
if b := s.endBlock(); b != nil {
|
||||
b.AddEdgeTo(bEnd)
|
||||
if n.Nbody.Len() != 0 {
|
||||
s.startBlock(bThen)
|
||||
s.stmtList(n.Nbody)
|
||||
if b := s.endBlock(); b != nil {
|
||||
b.AddEdgeTo(bEnd)
|
||||
}
|
||||
}
|
||||
|
||||
if n.Rlist.Len() != 0 {
|
||||
s.startBlock(bElse)
|
||||
s.stmtList(n.Rlist)
|
||||
|
|
|
@ -33,9 +33,7 @@ func f1(a [256]int, i int) {
|
|||
|
||||
if 4 <= i && i < len(a) {
|
||||
useInt(a[i])
|
||||
useInt(a[i-1]) // ERROR "Found IsInBounds$"
|
||||
// TODO: 'if 4 <= i && i < len(a)' gets rewritten to 'if uint(i - 4) < 256 - 4',
|
||||
// which the bounds checker cannot yet use to infer that the next line doesn't need a bounds check.
|
||||
useInt(a[i-1])
|
||||
useInt(a[i-4])
|
||||
}
|
||||
}
|
||||
|
|
23
test/fixedbugs/issue30566a.go
Normal file
23
test/fixedbugs/issue30566a.go
Normal file
|
@ -0,0 +1,23 @@
|
|||
// run
|
||||
|
||||
// Copyright 2019 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 main
|
||||
|
||||
import "fmt"
|
||||
|
||||
//go:noinline
|
||||
func ident(s string) string { return s }
|
||||
|
||||
func returnSecond(x bool, s string) string { return s }
|
||||
|
||||
func identWrapper(s string) string { return ident(s) }
|
||||
|
||||
func main() {
|
||||
got := returnSecond((false || identWrapper("bad") != ""), ident("good"))
|
||||
if got != "good" {
|
||||
panic(fmt.Sprintf("wanted \"good\", got \"%s\"", got))
|
||||
}
|
||||
}
|
27
test/fixedbugs/issue30566b.go
Normal file
27
test/fixedbugs/issue30566b.go
Normal file
|
@ -0,0 +1,27 @@
|
|||
// run
|
||||
|
||||
// Copyright 2019 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 main
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"fmt"
|
||||
)
|
||||
|
||||
func main() {
|
||||
_, _ = false || g(1), g(2)
|
||||
if !bytes.Equal(x, []byte{1, 2}) {
|
||||
panic(fmt.Sprintf("wanted [1,2], got %v", x))
|
||||
}
|
||||
}
|
||||
|
||||
var x []byte
|
||||
|
||||
//go:noinline
|
||||
func g(b byte) bool {
|
||||
x = append(x, b)
|
||||
return false
|
||||
}
|
|
@ -572,7 +572,7 @@ func f36() {
|
|||
func f37() {
|
||||
if (m33[byteptr()] == 0 || // ERROR "stack object .autotmp_[0-9]+ interface \{\}"
|
||||
m33[byteptr()] == 0) && // ERROR "stack object .autotmp_[0-9]+ interface \{\}"
|
||||
m33[byteptr()] == 0 { // ERROR "stack object .autotmp_[0-9]+ interface \{\}"
|
||||
m33[byteptr()] == 0 {
|
||||
printnl()
|
||||
return
|
||||
}
|
||||
|
@ -697,9 +697,10 @@ func f41(p, q *int) (r *int) { // ERROR "live at entry to f41: p q$"
|
|||
|
||||
func f42() {
|
||||
var p, q, r int
|
||||
f43([]*int{&p,&q,&r}) // ERROR "stack object .autotmp_[0-9]+ \[3\]\*int$"
|
||||
f43([]*int{&p,&r,&q})
|
||||
f43([]*int{&q,&p,&r})
|
||||
f43([]*int{&p, &q, &r}) // ERROR "stack object .autotmp_[0-9]+ \[3\]\*int$"
|
||||
f43([]*int{&p, &r, &q})
|
||||
f43([]*int{&q, &p, &r})
|
||||
}
|
||||
|
||||
//go:noescape
|
||||
func f43(a []*int)
|
||||
|
|
Loading…
Reference in a new issue