From 6317f92f6e51f679712deec6094c6b5fc2948a5b Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Sun, 27 Nov 2016 10:41:37 -0800 Subject: [PATCH] cmd/compile: fix CSE with commutative ops CSE opportunities were being missed for commutative ops. We used to order the args of commutative ops (by arg ID) once at the start of CSE. But that may not be enough. i1 = (Load ptr mem) i2 = (Load ptr mem) x1 = (Add i1 j) x2 = (Add i2 j) Equivalent commutative ops x1 and x2 may not get their args ordered in the same way because because at the start of CSE, we don't know that the i values will be CSEd. If x1 and x2 get opposite orders we won't CSE them. Instead, (re)order the args of commutative operations by their equivalence class IDs each time we partition an equivalence class. Change-Id: Ic609fa83b85299782a5e85bf93dc6023fccf4b0c Reviewed-on: https://go-review.googlesource.com/33632 Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot Reviewed-by: Todd Neal --- src/cmd/compile/internal/ssa/cse.go | 14 ++++++++++---- test/prove.go | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/cmd/compile/internal/ssa/cse.go b/src/cmd/compile/internal/ssa/cse.go index 4e07c89b888..39861b6e2a5 100644 --- a/src/cmd/compile/internal/ssa/cse.go +++ b/src/cmd/compile/internal/ssa/cse.go @@ -39,10 +39,6 @@ func cse(f *Func) { if v.Type.IsMemory() { continue // memory values can never cse } - if opcodeTable[v.Op].commutative && len(v.Args) == 2 && v.Args[1].ID < v.Args[0].ID { - // Order the arguments of binary commutative operations. - v.Args[0], v.Args[1] = v.Args[1], v.Args[0] - } a = append(a, v) } } @@ -92,6 +88,15 @@ func cse(f *Func) { for i := 0; i < len(partition); i++ { e := partition[i] + if opcodeTable[e[0].Op].commutative { + // Order the first two args before comparison. + for _, v := range e { + if valueEqClass[v.Args[0].ID] > valueEqClass[v.Args[1].ID] { + v.Args[0], v.Args[1] = v.Args[1], v.Args[0] + } + } + } + // Sort by eq class of arguments. byArgClass.a = e byArgClass.eqClass = valueEqClass @@ -101,6 +106,7 @@ func cse(f *Func) { splitPoints = append(splitPoints[:0], 0) for j := 1; j < len(e); j++ { v, w := e[j-1], e[j] + // Note: commutative args already correctly ordered by byArgClass. eqArgs := true for k, a := range v.Args { b := w.Args[k] diff --git a/test/prove.go b/test/prove.go index 9ced6166e03..9ef8949e1c4 100644 --- a/test/prove.go +++ b/test/prove.go @@ -273,7 +273,7 @@ func f11c(a []int, i int) { func f11d(a []int, i int) { useInt(a[2*i+7]) - useInt(a[2*i+7]) + useInt(a[2*i+7]) // ERROR "Proved boolean IsInBounds$" } func f12(a []int, b int) { @@ -438,6 +438,19 @@ func f13i(a uint) int { return 3 } +func f14(p, q *int, a []int) { + // This crazy ordering usually gives i1 the lowest value ID, + // j the middle value ID, and i2 the highest value ID. + // That used to confuse CSE because it ordered the args + // of the two + ops below differently. + // That in turn foiled bounds check elimination. + i1 := *p + j := *q + i2 := *p + useInt(a[i1+j]) + useInt(a[i2+j]) // ERROR "Proved boolean IsInBounds$" +} + //go:noinline func useInt(a int) { }