From ac743dea8e7d351d48a79bf4e7aea62ba3b4a515 Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Tue, 9 Jun 2020 03:17:17 -0700 Subject: [PATCH] cmd/compile: always tighten and de-duplicate tuple selectors The scheduler assumes two special invariants that apply to tuple selectors (Select0 and Select1 ops): 1. There is only one tuple selector of each type per generator. 2. Tuple selectors and generators reside in the same block. Prior to this CL the assumption was that these invariants would only be broken by the CSE pass. The CSE pass therefore contained code to move and de-duplicate selectors to fix these invariants. However it is also possible to write relatively basic optimization rules that cause these invariants to be broken. For example: (A (Select0 (B))) -> (Select1 (B)) This rule could result in the newly added selector (Select1) being in a different block to the tuple generator (see issue #38356). It could also result in duplicate selectors if this rule matches multiple times for the same tuple generator (see issue #39472). The CSE pass will 'fix' these invariants. However it will only do so when optimizations are enabled (since disabling optimizations disables the CSE pass). This CL moves the CSE tuple selector fixup code into its own pass and makes it mandatory even when optimizations are disabled. This allows tuple selectors to be treated like normal ops for most of the compilation pipeline until after the new pass has run, at which point we need to be careful to maintain the invariant again. Fixes #39472. Change-Id: Ia3f79e09d9c65ac95f897ce37e967ee1258a080b Reviewed-on: https://go-review.googlesource.com/c/go/+/237118 Run-TryBot: Michael Munday TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall --- src/cmd/compile/internal/ssa/compile.go | 3 ++ src/cmd/compile/internal/ssa/cse.go | 54 ---------------------- src/cmd/compile/internal/ssa/tuple.go | 59 +++++++++++++++++++++++++ test/fixedbugs/issue39472.go | 12 +++++ 4 files changed, 74 insertions(+), 54 deletions(-) create mode 100644 src/cmd/compile/internal/ssa/tuple.go create mode 100644 test/fixedbugs/issue39472.go diff --git a/src/cmd/compile/internal/ssa/compile.go b/src/cmd/compile/internal/ssa/compile.go index 2dbe9cf4052..dbdd027716a 100644 --- a/src/cmd/compile/internal/ssa/compile.go +++ b/src/cmd/compile/internal/ssa/compile.go @@ -451,6 +451,7 @@ var passes = [...]pass{ {name: "lowered deadcode for cse", fn: deadcode}, // deadcode immediately before CSE avoids CSE making dead values live again {name: "lowered cse", fn: cse}, {name: "elim unread autos", fn: elimUnreadAutos}, + {name: "tighten tuple selectors", fn: tightenTupleSelectors, required: true}, {name: "lowered deadcode", fn: deadcode, required: true}, {name: "checkLower", fn: checkLower, required: true}, {name: "late phielim", fn: phielim}, @@ -509,6 +510,8 @@ var passOrder = [...]constraint{ {"decompose builtin", "late opt"}, // decompose builtin is the last pass that may introduce new float ops, so run softfloat after it {"decompose builtin", "softfloat"}, + // tuple selectors must be tightened to generators and de-duplicated before scheduling + {"tighten tuple selectors", "schedule"}, // remove critical edges before phi tighten, so that phi args get better placement {"critical", "phi tighten"}, // don't layout blocks until critical edges have been removed diff --git a/src/cmd/compile/internal/ssa/cse.go b/src/cmd/compile/internal/ssa/cse.go index 6bfd2960c18..3b4f2be37e7 100644 --- a/src/cmd/compile/internal/ssa/cse.go +++ b/src/cmd/compile/internal/ssa/cse.go @@ -223,60 +223,6 @@ func cse(f *Func) { } } - // Fixup tuple selectors. - // - // If we have rewritten a tuple generator to a new one in a different - // block, copy its selectors to the new generator's block, so tuple - // generator and selectors stay together. - // - // Note: that there must be only one selector of each type per tuple - // generator. CSE may have left us with more than one so we de-duplicate - // them using a map. See issue 16741. - selectors := make(map[struct { - id ID - op Op - }]*Value) - for _, b := range f.Blocks { - for _, selector := range b.Values { - if selector.Op != OpSelect0 && selector.Op != OpSelect1 { - continue - } - - // Get the tuple generator to use as a key for de-duplication. - tuple := selector.Args[0] - if !tuple.Type.IsTuple() { - f.Fatalf("arg of tuple selector %s is not a tuple: %s", selector.String(), tuple.LongString()) - } - - // If there is a pre-existing selector in the target block then - // use that. Do this even if the selector is already in the - // target block to avoid duplicate tuple selectors. - key := struct { - id ID - op Op - }{tuple.ID, selector.Op} - if t := selectors[key]; t != nil { - if selector != t { - selector.copyOf(t) - } - continue - } - - // If the selector is in the wrong block copy it into the target - // block. - if selector.Block != tuple.Block { - t := selector.copyInto(tuple.Block) - selector.copyOf(t) - selectors[key] = t - continue - } - - // The selector is in the target block. Add it to the map so it - // cannot be duplicated. - selectors[key] = selector - } - } - if f.pass.stats > 0 { f.LogStat("CSE REWRITES", rewrites) } diff --git a/src/cmd/compile/internal/ssa/tuple.go b/src/cmd/compile/internal/ssa/tuple.go new file mode 100644 index 00000000000..38deabf83d2 --- /dev/null +++ b/src/cmd/compile/internal/ssa/tuple.go @@ -0,0 +1,59 @@ +// 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. + +package ssa + +// tightenTupleSelectors ensures that tuple selectors (Select0 and +// Select1 ops) are in the same block as their tuple generator. The +// function also ensures that there are no duplicate tuple selectors. +// These properties are expected by the scheduler but may not have +// been maintained by the optimization pipeline up to this point. +// +// See issues 16741 and 39472. +func tightenTupleSelectors(f *Func) { + selectors := make(map[struct { + id ID + op Op + }]*Value) + for _, b := range f.Blocks { + for _, selector := range b.Values { + if selector.Op != OpSelect0 && selector.Op != OpSelect1 { + continue + } + + // Get the tuple generator to use as a key for de-duplication. + tuple := selector.Args[0] + if !tuple.Type.IsTuple() { + f.Fatalf("arg of tuple selector %s is not a tuple: %s", selector.String(), tuple.LongString()) + } + + // If there is a pre-existing selector in the target block then + // use that. Do this even if the selector is already in the + // target block to avoid duplicate tuple selectors. + key := struct { + id ID + op Op + }{tuple.ID, selector.Op} + if t := selectors[key]; t != nil { + if selector != t { + selector.copyOf(t) + } + continue + } + + // If the selector is in the wrong block copy it into the target + // block. + if selector.Block != tuple.Block { + t := selector.copyInto(tuple.Block) + selector.copyOf(t) + selectors[key] = t + continue + } + + // The selector is in the target block. Add it to the map so it + // cannot be duplicated. + selectors[key] = selector + } + } +} diff --git a/test/fixedbugs/issue39472.go b/test/fixedbugs/issue39472.go new file mode 100644 index 00000000000..61444a28b91 --- /dev/null +++ b/test/fixedbugs/issue39472.go @@ -0,0 +1,12 @@ +// compile -N + +// 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. + +package p + +func f(x float64) bool { + x += 1 + return (x != 0) == (x != 0) +}