Skip to content

Commit 256210c

Browse files
randall77randall77
committed
cmd/compile: better check for single live memory
Enhance the one-live-memory-at-a-time check to run during many more phases of the SSA backend. Also make it work in an interblock fashion. Change types.IsMemory to return true for tuples containing a memory type. Fix trim pass to build the merged phi correctly. Doesn't affect code but allows the check to pass after trim runs. Switch the AddTuple* ops to take the memory-containing tuple argument second. Update #20335 Change-Id: I5b03ef3606b75a9e4f765276bb8b183cdc172b43 Reviewed-on: https://go-review.googlesource.com/43495 Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Cherry Zhang <[email protected]>
1 parent d5e01c0 commit 256210c

13 files changed

+189
-94
lines changed

‎src/cmd/compile/internal/ssa/check.go

Lines changed: 114 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,10 @@ func checkFunc(f *Func) {
310310
}
311311
}
312312

313+
memCheck(f)
314+
}
315+
316+
func memCheck(f *Func) {
313317
// Check that if a tuple has a memory type, it is second.
314318
for _, b := range f.Blocks {
315319
for _, v := range b.Values {
@@ -319,24 +323,122 @@ func checkFunc(f *Func) {
319323
}
320324
}
321325

326+
// Single live memory checks.
327+
// These checks only work if there are no memory copies.
328+
// (Memory copies introduce ambiguity about which mem value is really live.
329+
// probably fixable, but it's easier to avoid the problem.)
330+
// For the same reason, disable this check if some memory ops are unused.
331+
for _, b := range f.Blocks {
332+
for _, v := range b.Values {
333+
if (v.Op == OpCopy || v.Uses == 0) && v.Type.IsMemory() {
334+
return
335+
}
336+
}
337+
if b != f.Entry && len(b.Preds) == 0 {
338+
return
339+
}
340+
}
341+
342+
// Compute live memory at the end of each block.
343+
lastmem := make([]*Value, f.NumBlocks())
344+
ss := newSparseSet(f.NumValues())
345+
for _, b := range f.Blocks {
346+
// Mark overwritten memory values. Those are args of other
347+
// ops that generate memory values.
348+
ss.clear()
349+
for _, v := range b.Values {
350+
if v.Op == OpPhi || !v.Type.IsMemory() {
351+
continue
352+
}
353+
if m := v.MemoryArg(); m != nil {
354+
ss.add(m.ID)
355+
}
356+
}
357+
// There should be at most one remaining unoverwritten memory value.
358+
for _, v := range b.Values {
359+
if !v.Type.IsMemory() {
360+
continue
361+
}
362+
if ss.contains(v.ID) {
363+
continue
364+
}
365+
if lastmem[b.ID] != nil {
366+
f.Fatalf("two live memory values in %s: %s and %s", b, lastmem[b.ID], v)
367+
}
368+
lastmem[b.ID] = v
369+
}
370+
// If there is no remaining memory value, that means there was no memory update.
371+
// Take any memory arg.
372+
if lastmem[b.ID] == nil {
373+
for _, v := range b.Values {
374+
if v.Op == OpPhi {
375+
continue
376+
}
377+
m := v.MemoryArg()
378+
if m == nil {
379+
continue
380+
}
381+
if lastmem[b.ID] != nil && lastmem[b.ID] != m {
382+
f.Fatalf("two live memory values in %s: %s and %s", b, lastmem[b.ID], m)
383+
}
384+
lastmem[b.ID] = m
385+
}
386+
}
387+
}
388+
// Propagate last live memory through storeless blocks.
389+
for {
390+
changed := false
391+
for _, b := range f.Blocks {
392+
if lastmem[b.ID] != nil {
393+
continue
394+
}
395+
for _, e := range b.Preds {
396+
p := e.b
397+
if lastmem[p.ID] != nil {
398+
lastmem[b.ID] = lastmem[p.ID]
399+
changed = true
400+
break
401+
}
402+
}
403+
}
404+
if !changed {
405+
break
406+
}
407+
}
408+
// Check merge points.
409+
for _, b := range f.Blocks {
410+
for _, v := range b.Values {
411+
if v.Op == OpPhi && v.Type.IsMemory() {
412+
for i, a := range v.Args {
413+
if a != lastmem[b.Preds[i].b.ID] {
414+
f.Fatalf("inconsistent memory phi %s %d %s %s", v.LongString(), i, a, lastmem[b.Preds[i].b.ID])
415+
}
416+
}
417+
}
418+
}
419+
}
420+
322421
// Check that only one memory is live at any point.
323-
// TODO: make this check examine interblock.
324422
if f.scheduled {
325423
for _, b := range f.Blocks {
326-
var mem *Value // the live memory
424+
var mem *Value // the current live memory in the block
327425
for _, v := range b.Values {
328-
if v.Op != OpPhi {
329-
for _, a := range v.Args {
330-
if a.Type.IsMemory() || a.Type.IsTuple() && a.Type.FieldType(1).IsMemory() {
331-
if mem == nil {
332-
mem = a
333-
} else if mem != a {
334-
f.Fatalf("two live mems @ %s: %s and %s", v, mem, a)
335-
}
336-
}
426+
if v.Op == OpPhi {
427+
if v.Type.IsMemory() {
428+
mem = v
429+
}
430+
continue
431+
}
432+
if mem == nil && len(b.Preds) > 0 {
433+
// If no mem phi, take mem of any predecessor.
434+
mem = lastmem[b.Preds[0].b.ID]
435+
}
436+
for _, a := range v.Args {
437+
if a.Type.IsMemory() && a != mem {
438+
f.Fatalf("two live mems @ %s: %s and %s", v, mem, a)
337439
}
338440
}
339-
if v.Type.IsMemory() || v.Type.IsTuple() && v.Type.FieldType(1).IsMemory() {
441+
if v.Type.IsMemory() {
340442
mem = v
341443
}
342444
}

‎src/cmd/compile/internal/ssa/deadstore.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,6 @@ func dse(f *Func) {
3434
}
3535
if v.Type.IsMemory() {
3636
stores = append(stores, v)
37-
if v.Op == OpSelect1 {
38-
// Use the args of the tuple-generating op.
39-
v = v.Args[0]
40-
}
4137
for _, a := range v.Args {
4238
if a.Block == b && a.Type.IsMemory() {
4339
storeUse.add(a.ID)

‎src/cmd/compile/internal/ssa/gen/AMD64.rules

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -487,12 +487,12 @@
487487
(AtomicExchange64 ptr val mem) -> (XCHGQ val ptr mem)
488488

489489
// Atomic adds.
490-
(AtomicAdd32 ptr val mem) -> (AddTupleFirst32 (XADDLlock val ptr mem) val)
491-
(AtomicAdd64 ptr val mem) -> (AddTupleFirst64 (XADDQlock val ptr mem) val)
492-
(Select0 <t> (AddTupleFirst32 tuple val)) -> (ADDL val (Select0 <t> tuple))
493-
(Select1 (AddTupleFirst32 tuple _ )) -> (Select1 tuple)
494-
(Select0 <t> (AddTupleFirst64 tuple val)) -> (ADDQ val (Select0 <t> tuple))
495-
(Select1 (AddTupleFirst64 tuple _ )) -> (Select1 tuple)
490+
(AtomicAdd32 ptr val mem) -> (AddTupleFirst32 val (XADDLlock val ptr mem))
491+
(AtomicAdd64 ptr val mem) -> (AddTupleFirst64 val (XADDQlock val ptr mem))
492+
(Select0 <t> (AddTupleFirst32 val tuple)) -> (ADDL val (Select0 <t> tuple))
493+
(Select1 (AddTupleFirst32 _ tuple)) -> (Select1 tuple)
494+
(Select0 <t> (AddTupleFirst64 val tuple)) -> (ADDQ val (Select0 <t> tuple))
495+
(Select1 (AddTupleFirst64 _ tuple)) -> (Select1 tuple)
496496

497497
// Atomic compare and swap.
498498
(AtomicCompareAndSwap32 ptr old new_ mem) -> (CMPXCHGLlock ptr old new_ mem)

‎src/cmd/compile/internal/ssa/gen/AMD64Ops.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -572,8 +572,8 @@ func init() {
572572
// Note: arg0 and arg1 are backwards compared to MOVLstore (to facilitate resultInArg0)!
573573
{name: "XADDLlock", argLength: 3, reg: gpstorexchg, asm: "XADDL", typ: "(UInt32,Mem)", aux: "SymOff", resultInArg0: true, clobberFlags: true, faultOnNilArg1: true, hasSideEffects: true, symEffect: "RdWr"},
574574
{name: "XADDQlock", argLength: 3, reg: gpstorexchg, asm: "XADDQ", typ: "(UInt64,Mem)", aux: "SymOff", resultInArg0: true, clobberFlags: true, faultOnNilArg1: true, hasSideEffects: true, symEffect: "RdWr"},
575-
{name: "AddTupleFirst32", argLength: 2}, // arg0=tuple <x,y>. Returns <x+arg1,y>.
576-
{name: "AddTupleFirst64", argLength: 2}, // arg0=tuple <x,y>. Returns <x+arg1,y>.
575+
{name: "AddTupleFirst32", argLength: 2}, // arg1=tuple <x,y>. Returns <x+arg0,y>.
576+
{name: "AddTupleFirst64", argLength: 2}, // arg1=tuple <x,y>. Returns <x+arg0,y>.
577577

578578
// Compare and swap.
579579
// arg0 = pointer, arg1 = old value, arg2 = new value, arg3 = memory.

‎src/cmd/compile/internal/ssa/gen/S390X.rules

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,12 @@
120120
(AtomicStorePtrNoWB ptr val mem) -> (MOVDatomicstore ptr val mem)
121121

122122
// Atomic adds.
123-
(AtomicAdd32 ptr val mem) -> (AddTupleFirst32 (LAA ptr val mem) val)
124-
(AtomicAdd64 ptr val mem) -> (AddTupleFirst64 (LAAG ptr val mem) val)
125-
(Select0 <t> (AddTupleFirst32 tuple val)) -> (ADDW val (Select0 <t> tuple))
126-
(Select1 (AddTupleFirst32 tuple _ )) -> (Select1 tuple)
127-
(Select0 <t> (AddTupleFirst64 tuple val)) -> (ADD val (Select0 <t> tuple))
128-
(Select1 (AddTupleFirst64 tuple _ )) -> (Select1 tuple)
123+
(AtomicAdd32 ptr val mem) -> (AddTupleFirst32 val (LAA ptr val mem))
124+
(AtomicAdd64 ptr val mem) -> (AddTupleFirst64 val (LAAG ptr val mem))
125+
(Select0 <t> (AddTupleFirst32 val tuple)) -> (ADDW val (Select0 <t> tuple))
126+
(Select1 (AddTupleFirst32 _ tuple)) -> (Select1 tuple)
127+
(Select0 <t> (AddTupleFirst64 val tuple)) -> (ADD val (Select0 <t> tuple))
128+
(Select1 (AddTupleFirst64 _ tuple)) -> (Select1 tuple)
129129

130130
// Atomic exchanges.
131131
(AtomicExchange32 ptr val mem) -> (LoweredAtomicExchange32 ptr val mem)

‎src/cmd/compile/internal/ssa/gen/S390XOps.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,8 +467,8 @@ func init() {
467467
// Returns a tuple of <old contents of *(arg0+auxint+aux), memory>.
468468
{name: "LAA", argLength: 3, reg: gpstorelaa, asm: "LAA", typ: "(UInt32,Mem)", aux: "SymOff", faultOnNilArg0: true, hasSideEffects: true, symEffect: "RdWr"},
469469
{name: "LAAG", argLength: 3, reg: gpstorelaa, asm: "LAAG", typ: "(UInt64,Mem)", aux: "SymOff", faultOnNilArg0: true, hasSideEffects: true, symEffect: "RdWr"},
470-
{name: "AddTupleFirst32", argLength: 2}, // arg0=tuple <x,y>. Returns <x+arg1,y>.
471-
{name: "AddTupleFirst64", argLength: 2}, // arg0=tuple <x,y>. Returns <x+arg1,y>.
470+
{name: "AddTupleFirst32", argLength: 2}, // arg1=tuple <x,y>. Returns <x+arg0,y>.
471+
{name: "AddTupleFirst64", argLength: 2}, // arg1=tuple <x,y>. Returns <x+arg0,y>.
472472

473473
// Compare and swap.
474474
// arg0 = pointer, arg1 = old value, arg2 = new value, arg3 = memory.

‎src/cmd/compile/internal/ssa/loopreschedchecks.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -391,10 +391,6 @@ func findLastMems(f *Func) []*Value {
391391
}
392392
if v.Type.IsMemory() {
393393
stores = append(stores, v)
394-
if v.Op == OpSelect1 {
395-
// Use the arg of the tuple-generating op.
396-
v = v.Args[0]
397-
}
398394
for _, a := range v.Args {
399395
if a.Block == b && a.Type.IsMemory() {
400396
storeUse.add(a.ID)

‎src/cmd/compile/internal/ssa/rewriteAMD64.go

Lines changed: 14 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)