[go] cmd/compile: add memory-less pure call and rewrite to static call
David Chase (Gerrit)
David Chase has uploaded this change for review.
cmd/compile: add memory-less pure call and rewrite to static call
When tested, this works!
Change-Id: I1bfdb6a05c7f6e78c4dc432cde36e9adb26bdc42
---
M src/cmd/compile/internal/ssa/_gen/genericOps.go
M src/cmd/compile/internal/ssa/compile.go
M src/cmd/compile/internal/ssa/deadcode.go
M src/cmd/compile/internal/ssa/expand_calls.go
A src/cmd/compile/internal/ssa/impurify.go
M src/cmd/compile/internal/ssa/op.go
M src/cmd/compile/internal/ssa/opGen.go
M src/cmd/compile/internal/ssagen/ssa.go
8 files changed, 397 insertions(+), 16 deletions(-)
diff --git a/src/cmd/compile/internal/ssa/_gen/genericOps.go b/src/cmd/compile/internal/ssa/_gen/genericOps.go
index a182afb..fc858e1 100644
--- a/src/cmd/compile/internal/ssa/_gen/genericOps.go
+++ b/src/cmd/compile/internal/ssa/_gen/genericOps.go
@@ -437,6 +437,8 @@
{name: "InterLECall", argLength: -1, aux: "CallOff", call: true}, // late-expanded interface call. arg0=code pointer, arg1..argN-1 are inputs, argN is mem. auxint = arg size. Result is tuple of result(s), plus mem.
{name: "TailLECall", argLength: -1, aux: "CallOff", call: true}, // late-expanded static tail call function aux.(*ssa.AuxCall.Fn). arg0..argN-1 are inputs, argN is mem. auxint = arg size. Result is tuple of result(s), plus mem.
+ {name: "PureLECall", argLength: -1, aux: "CallOff", call: true}, // late-expanded static call function aux.(*ssa.AuxCall.Fn). arg0..argN are inputs, there is NO MEM. auxint = arg size. Result is tuple of result(s),and NO MEM.
+
// Conversions: signed extensions, zero (unsigned) extensions, truncations
{name: "SignExt8to16", argLength: 1, typ: "Int16"},
{name: "SignExt8to32", argLength: 1, typ: "Int32"},
diff --git a/src/cmd/compile/internal/ssa/compile.go b/src/cmd/compile/internal/ssa/compile.go
index d125891..50f9117 100644
--- a/src/cmd/compile/internal/ssa/compile.go
+++ b/src/cmd/compile/internal/ssa/compile.go
@@ -472,6 +472,7 @@
{name: "nilcheckelim", fn: nilcheckelim},
{name: "prove", fn: prove},
{name: "early fuse", fn: fuseEarly},
+ {name: "impurify", fn: impurifyCalls, required: true},
{name: "expand calls", fn: expandCalls, required: true},
{name: "decompose builtin", fn: postExpandCallsDecompose, required: true},
{name: "softfloat", fn: softfloat, required: true},
diff --git a/src/cmd/compile/internal/ssa/deadcode.go b/src/cmd/compile/internal/ssa/deadcode.go
index 52cc7f2..65cd432 100644
--- a/src/cmd/compile/internal/ssa/deadcode.go
+++ b/src/cmd/compile/internal/ssa/deadcode.go
@@ -110,7 +110,7 @@
}
}
for _, v := range b.Values {
- if (opcodeTable[v.Op].call || opcodeTable[v.Op].hasSideEffects) && !live[v.ID] {
+ if (opcodeTable[v.Op].call && v.Op != OpPureLECall || opcodeTable[v.Op].hasSideEffects) && !live[v.ID] {
live[v.ID] = true
q = append(q, v)
if v.Pos.IsStmt() != src.PosNotStmt {
diff --git a/src/cmd/compile/internal/ssa/expand_calls.go b/src/cmd/compile/internal/ssa/expand_calls.go
index 29c180b..128cac3 100644
--- a/src/cmd/compile/internal/ssa/expand_calls.go
+++ b/src/cmd/compile/internal/ssa/expand_calls.go
@@ -70,6 +70,8 @@
case OpInitMem:
m0 = v
+ case OpPureLECall:
+
case OpClosureLECall, OpInterLECall, OpStaticLECall, OpTailLECall:
calls = append(calls, v)
@@ -109,8 +111,8 @@
off := x.offsetFrom(x.f.Entry, x.sp, aux.OffsetOfResult(which), pt)
v.copyOf(off)
}
- }
+ }
// rewrite function results from an exit block
// values returned by function need to be split out into registers.
if isBlockMultiValueExit(b) {
diff --git a/src/cmd/compile/internal/ssa/impurify.go b/src/cmd/compile/internal/ssa/impurify.go
new file mode 100644
index 0000000..24828d9
--- /dev/null
+++ b/src/cmd/compile/internal/ssa/impurify.go
@@ -0,0 +1,315 @@
+// Copyright 2023 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
+
+import (
+ "cmd/compile/internal/types"
+ "cmd/internal/src"
+ "fmt"
+)
+
+// impurifyCalls converts "pure" calls that take/produce no memory operand
+// to static calls that do.
+func impurifyCalls(f *Func) {
+ debug := f.pass.debug
+ po := f.postorder()
+
+ // Convert pure functions to regular functions, rewriting mem as necessary.
+ lastMems := make([]*Value, f.NumBlocks()) // for a block, what is the last visible mem?
+ firstMems := make([]*Value, f.NumBlocks()) // for a block, what is the Preds[0] mem?
+ memPhi := make([]*Value, f.NumBlocks()) // for a block, what is its mem phi function (if any)
+ visited := make([]bool, f.NumBlocks())
+ anyChanged := false
+
+ // Topological sort stuff
+ dependsOn := make([]int, f.NumValues())
+ dependents := make([][]*Value, f.NumValues())
+ var zeroDeps []*Value
+ var zeroMemDeps []*Value
+ var zeroMemDepsCall []*Value
+ var lastMem *Value
+
+ // Sort values into dependence/store order
+ // Keep track of mem start and end values, and memory phi functions for blocks.
+ // TODO optimize later if any of this is a performance problem.
+ for j := len(po) - 1; j >= 0; j-- {
+ b := po[j]
+ anyChanged = false
+ lastMem = nil
+
+ if debug > 0 {
+ fmt.Printf("PO visit b%d\n", b.ID)
+ }
+
+ // Figure out mem exposed at top of block, if any.
+ if l := len(b.Preds); l > 0 {
+ // nil if there is a difference
+ for _, p := range b.Preds {
+ pbid := p.Block().ID
+ if debug > 0 {
+ fmt.Printf("\tpbid=%d, visited=%v, lastMems[]=%s\n", pbid, visited[pbid], lastMems[pbid].LongString())
+ }
+ if visited[pbid] {
+ if lastMem == nil {
+ lastMem = lastMems[pbid]
+ firstMems[b.ID] = lastMem // Could be nil, only matters for single predecessor case.
+ } else if lastMems[pbid] != lastMem {
+ lastMem = nil
+ break
+ }
+ }
+ }
+ }
+
+ if debug > 0 {
+ fmt.Printf("\tlastMem=%s\n", lastMem.LongString())
+ }
+
+ visited[b.ID] = true
+
+ zeroDeps = zeroDeps[:0]
+ zeroMemDeps = zeroMemDeps[:0]
+ zeroMemDepsCall = zeroMemDepsCall[:0]
+
+ isCallOp := func(o Op) bool {
+ return o == OpStaticLECall || o == OpClosureLECall || o == OpInterLECall || o == OpTailLECall || o == OpPureLECall
+ }
+
+ // Append v to the proper list of depends-on-nothing.
+ appendZeroDeps := func(v *Value) {
+ if v.Type != types.TypeMem && v.Op != OpPureLECall {
+ zeroDeps = append(zeroDeps, v)
+ return
+ }
+
+ if isCallOp(v.Op) {
+ zeroMemDepsCall = append(zeroMemDepsCall, v)
+ return
+ }
+ zeroMemDeps = append(zeroMemDeps, v)
+
+ }
+
+ // Set up for topological order, note memory-typed Phi and InitMem
+ for _, v := range b.Values {
+ switch v.Op {
+ case OpPhi:
+ if v.Type == types.TypeMem {
+ memPhi[b.ID] = v
+ lastMem = v
+ }
+ zeroDeps = append(zeroDeps, v) // Phis come first, even the memory phis.
+ continue
+
+ case OpInitMem:
+ lastMem = v
+ zeroMemDeps = append(zeroMemDeps, v)
+ continue
+ }
+ d := 0
+ for _, a := range v.Args {
+ if a.Block != b {
+ continue
+ }
+ dependents[a.ID] = append(dependents[a.ID], v)
+ d++
+ }
+ if d == 0 {
+ appendZeroDeps(v)
+ } else {
+ dependsOn[v.ID] = d
+ }
+ }
+
+ changed := false
+ b.Values = b.Values[:0]
+
+ if lastMem == nil { // no mem phi function, no InitMem, predecessors have varying mems, a phi is necessary.
+ // Create a new memory phi; if there are any backedge inputs, use self for their value; that is both a marker and a best-guess.
+ changed = true // if there is not a later mem output, this one will appear at the end and it is new.
+ newPhi := b.NewValue0(src.NoXPos, OpPhi, types.TypeMem)
+ for _, p := range b.Preds {
+ pbid := p.Block().ID
+ m := lastMems[pbid]
+ if m == nil {
+ m = newPhi
+ }
+ newPhi.AddArg(m)
+ }
+ if debug > 0 {
+ fmt.Printf("\tb%d, lastMem == nil adding mem phi %s\n", b.ID, newPhi.LongString())
+ }
+
+ memPhi[b.ID] = newPhi
+ lastMem = newPhi
+ }
+
+ // TODO use slices.Reverse(zeroDeps) when the bootstrap compiler supports it.
+ reverse := func(zd []*Value) {
+ l := len(zd)
+ for i := 0; i < l/2; i++ {
+ zd[i], zd[l-1-i] = zd[l-1-i], zd[i]
+ }
+ }
+ reverse(zeroDeps)
+ reverse(zeroMemDeps)
+ reverse(zeroMemDepsCall)
+
+ getOneZeroDep := func(p *[]*Value) *Value {
+ s := *p
+ if l := len(s); l > 0 {
+ z := s[l-1]
+ *p = s[:l-1]
+ b.Values = append(b.Values, z)
+ return z
+ }
+ return nil
+ }
+
+ getZeroDep := func() *Value {
+ // Don't release another mem until everything depending on the previous mem has been scheduled.
+ // Because of updates to other mems, getting this wrong will change memory operands and order.
+ if v := getOneZeroDep(&zeroDeps); v != nil {
+ return v
+ }
+ if v := getOneZeroDep(&zeroMemDeps); v != nil {
+ return v
+ }
+ return getOneZeroDep(&zeroMemDepsCall)
+ }
+
+ // run a topological order on the values, and rewrite pure calls as the sort goes by.
+ for z := getZeroDep(); z != nil; z = getZeroDep() {
+
+ if z.Op != OpPhi {
+ // If z has a memory operand and it is not lastMem, change it.
+ if m := z.MemoryArg(); m != nil && m != lastMem && m.Type == types.TypeMem {
+ z.SetArg(len(z.Args)-1, lastMem)
+ }
+ }
+
+ if z.Type == types.TypeMem {
+ changed = false // this output hides any new ones preceding it.
+ lastMem = z
+ }
+
+ for _, v := range dependents[z.ID] {
+ i := v.ID
+ dependsOn[i]--
+ if dependsOn[i] == 0 {
+ appendZeroDeps(v)
+ }
+ }
+ // Do this rewrite after updating dependsOn/zeroDeps
+ if z.Op == OpPureLECall {
+ // Translate into OpStaticLECall and insert an OpSelectN to extract the new memory.
+ z.Op = OpStaticLECall
+ z.AddArg(lastMem)
+ auxCall := z.Aux.(*AuxCall)
+ nresults := auxCall.NResults()
+ z.Type = auxCall.LateExpansionResultType()
+ lastMem = b.NewValue1I(z.Pos.WithNotStmt(), OpSelectN, types.TypeMem, nresults, z)
+ changed = true
+ }
+
+ // TODO slice reuse for dependents is a likely optimization
+ // possible hack -- use block value index instead of *Value/v.ID
+ dependents[z.ID] = nil
+ }
+
+ if debug > 0 {
+ fmt.Printf("\tb%d, lastMem=%s, changed=%v\n", b.ID, lastMem.LongString(), changed)
+ }
+ lastMems[b.ID] = lastMem
+ if changed {
+ anyChanged = true
+ }
+ }
+
+ // Perhaps we could be clever-er about propagating change, but first, need to see if it is useful.
+ for anyChanged {
+ if debug > 0 {
+ fmt.Printf("anyChanged loop\n")
+ }
+ anyChanged = false
+ for j := len(po) - 1; j >= 0; j-- {
+ b := po[j]
+ l := len(b.Preds)
+ if l == 0 {
+ continue
+ }
+
+ lastMem := lastMems[b.Preds[0].Block().ID]
+
+ // First figure out if the memory flowing into the non-phi part of the block has changed,
+ // and/or if a new phi function is required.
+ if l == 1 {
+ if lastMem == firstMems[b.ID] {
+ // no change
+ continue
+ }
+ firstMems[b.ID] = lastMem
+ if lastMems[b.ID].Block != b { // it is a flow-through block, successors will see the change.
+ anyChanged = true
+ lastMems[b.ID] = lastMem
+ }
+ } else if l > 1 {
+ if p := memPhi[b.ID]; p != nil {
+ for i, v := range p.Args {
+ lm := lastMems[b.Preds[i].Block().ID]
+ if v != lm {
+ p.SetArg(i, lm)
+ }
+ }
+ continue
+ }
+
+ // there's no phi function here, be sure that is the right choice.
+ // nil if there is a difference
+ for _, p := range b.Preds[1:] {
+ pbid := p.Block().ID
+ if lastMems[pbid] != lastMem {
+ lastMem = nil
+ break
+ }
+ }
+ if lastMem != nil {
+ continue
+ }
+ // need to insert a new phi function and correct memory inputs.
+ newPhi := b.NewValue0(src.NoXPos, OpPhi, types.TypeMem)
+ if lastMems[b.ID].Block != b { // it was a flow-through block and successors will see the change.
+ anyChanged = true
+ lastMems[b.ID] = newPhi
+ }
+ for _, p := range b.Preds {
+ newPhi.AddArg(lastMems[p.Block().ID])
+ }
+ lastMem = newPhi
+ if debug > 0 {
+ fmt.Printf("\tb%d, changed, adding mem phi %s\n", b.ID, newPhi.LongString())
+ }
+ memPhi[b.ID] = newPhi
+ }
+
+ // If we've not continued the loop, that means a new memory
+ // is present at the top of this block and needs to be copied
+ // into any uses.
+ for _, v := range b.Values {
+ if v.Op == OpPhi {
+ continue
+ }
+ // If z has a true memory operand (not a tuple) and it is not lastMem, change it.
+ if m := v.MemoryArg(); m != nil && m.Type == types.TypeMem && m != lastMem {
+ v.SetArg(len(v.Args)-1, lastMem)
+ }
+ if v.Type == types.TypeMem {
+ // because they're in dependence order, can quit early
+ break
+ }
+ }
+ }
+ }
+}
diff --git a/src/cmd/compile/internal/ssa/op.go b/src/cmd/compile/internal/ssa/op.go
index cb151b2..223aad0 100644
--- a/src/cmd/compile/internal/ssa/op.go
+++ b/src/cmd/compile/internal/ssa/op.go
@@ -273,11 +273,21 @@
// LateExpansionResultType returns the result type (including trailing mem)
// for a call that will be expanded later in the SSA phase.
func (a *AuxCall) LateExpansionResultType() *types.Type {
+ return a.LateExpansionResultTypeOptMem(true)
+}
+
+// LateExpansionResultTypeOptMem returns the result type
+// for a call that will be expanded later in the SSA phase.
+// Input parameter addMem specifies whether a mem type should
+// be added
+func (a *AuxCall) LateExpansionResultTypeOptMem(addMem bool) *types.Type {
var tys []*types.Type
for i := int64(0); i < a.NResults(); i++ {
tys = append(tys, a.TypeOfResult(i))
}
- tys = append(tys, types.TypeMem)
+ if addMem {
+ tys = append(tys, types.TypeMem)
+ }
return types.NewResults(tys)
}
diff --git a/src/cmd/compile/internal/ssa/opGen.go b/src/cmd/compile/internal/ssa/opGen.go
index 5640483..ef2ada3 100644
--- a/src/cmd/compile/internal/ssa/opGen.go
+++ b/src/cmd/compile/internal/ssa/opGen.go
@@ -3080,6 +3080,7 @@
OpStaticLECall
OpInterLECall
OpTailLECall
+ OpPureLECall
OpSignExt8to16
OpSignExt8to32
OpSignExt8to64
@@ -39609,6 +39610,13 @@
generic: true,
},
{
+ name: "PureLECall",
+ auxType: auxCallOff,
+ argLen: -1,
+ call: true,
+ generic: true,
+ },
+ {
name: "SignExt8to16",
argLen: 1,
generic: true,
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index af2e0e4..af4b28e 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -3233,7 +3233,7 @@
case ir.ORESULT:
n := n.(*ir.ResultExpr)
- if s.prevCall == nil || s.prevCall.Op != ssa.OpStaticLECall && s.prevCall.Op != ssa.OpInterLECall && s.prevCall.Op != ssa.OpClosureLECall {
+ if s.prevCall == nil || s.prevCall.Op != ssa.OpStaticLECall && s.prevCall.Op != ssa.OpPureLECall && s.prevCall.Op != ssa.OpInterLECall && s.prevCall.Op != ssa.OpClosureLECall {
panic("Expected to see a previous call")
}
which := n.Index
@@ -5321,27 +5321,64 @@
// Returns the address of the return value (or nil if none).
func (s *state) call(n *ir.CallExpr, k callKind, returnResultAddr bool, deferExtra ir.Expr) *ssa.Value {
s.prevCall = nil
- var callee *ir.Name // target function (if static)
- var closure *ssa.Value // ptr to closure to run (if dynamic)
- var codeptr *ssa.Value // ptr to target code (if dynamic)
- var dextra *ssa.Value // defer extra arg
- var rcvr *ssa.Value // receiver to set
+ var calleeLSym *obj.LSym // target function (if static)
+ var closure *ssa.Value // ptr to closure to run (if dynamic)
+ var codeptr *ssa.Value // ptr to target code (if dynamic)
+ var dextra *ssa.Value // defer extra arg
+ var rcvr *ssa.Value // receiver to set
fn := n.Fun
var ACArgs []*types.Type // AuxCall args
var ACResults []*types.Type // AuxCall results
var callArgs []*ssa.Value // For late-expansion, the args themselves (not stored, args to the call instead).
+ impure := true
+
callABI := s.f.ABIDefault
if k != callNormal && k != callTail && (len(n.Args) != 0 || n.Op() == ir.OCALLINTER || n.Fun.Type().NumResults() != 0) {
s.Fatalf("go/defer call with arguments: %v", n)
}
+ canBePure := func(s *obj.LSym) bool {
+ p := s.Pkg
+ if p == "" {
+ return false
+ }
+ return false // TODO choose pure functions.
+
+ // Sample implementation for "math functions are pure"
+ // l := len(p)
+ // if !strings.HasPrefix(p, "math") {
+ // return false
+ // }
+ // if l > len("math") && p[len("math")] != '/' {
+ // return false
+ // }
+ // if p == "math/rand" {
+ // return false
+ // }
+ // n := s.Name
+ // if n == "" {
+ // return false
+ // }
+ // n = n[l+1:]
+ // if !unicode.IsUpper(rune(n[0])) {
+ // return false
+ // }
+ // if strings.HasPrefix(n, "Test") || strings.HasPrefix(n, "Benchmark") || strings.HasPrefix(n, "Example") {
+ // return false
+ // }
+ // return true
+ }
+
switch n.Op() {
case ir.OCALLFUNC:
if (k == callNormal || k == callTail) && fn.Op() == ir.ONAME && fn.(*ir.Name).Class == ir.PFUNC {
fn := fn.(*ir.Name)
- callee = fn
+ calleeLSym = callTargetLSym(fn)
+ if s.f.DebugHashMatch() {
+ impure = !(k == callNormal && canBePure(calleeLSym))
+ }
if buildcfg.Experiment.RegabiArgs {
// This is a static call, so it may be
// a direct call to a non-ABIInternal
@@ -5462,7 +5499,9 @@
callArgs = append(callArgs, s.putArg(n, t.Param(i).Type))
}
- callArgs = append(callArgs, s.mem())
+ if impure {
+ callArgs = append(callArgs, s.mem())
+ }
// call target
switch {
@@ -5489,10 +5528,12 @@
// Note that the "receiver" parameter is nil because the actual receiver is the first input parameter.
aux := ssa.InterfaceAuxCall(params)
call = s.newValue1A(ssa.OpInterLECall, aux.LateExpansionResultType(), aux, codeptr)
- case callee != nil:
- aux := ssa.StaticAuxCall(callTargetLSym(callee), params)
- call = s.newValue0A(ssa.OpStaticLECall, aux.LateExpansionResultType(), aux)
- if k == callTail {
+ case calleeLSym != nil:
+ aux := ssa.StaticAuxCall(calleeLSym, params)
+ call = s.newValue0A(ssa.OpStaticLECall, aux.LateExpansionResultTypeOptMem(impure), aux)
+ if !impure {
+ call.Op = ssa.OpPureLECall
+ } else if k == callTail {
call.Op = ssa.OpTailLECall
stksize = 0 // Tail call does not use stack. We reuse caller's frame.
}
@@ -5503,7 +5544,9 @@
call.AuxInt = stksize // Call operations carry the argsize of the callee along with them
}
s.prevCall = call
- s.vars[memVar] = s.newValue1I(ssa.OpSelectN, types.TypeMem, int64(len(ACResults)), call)
+ if impure {
+ s.vars[memVar] = s.newValue1I(ssa.OpSelectN, types.TypeMem, int64(len(ACResults)), call)
+ }
// Insert VarLive opcodes.
for _, v := range n.KeepAlive {
if !v.Addrtaken() {
To view, visit change 533265. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: David Chase, Keith Randall, Martin Möhrmann, Matthew Dempsky.
David Chase uploaded patch set #2 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by David Chase, TryBot-Result-1 by Gopher Robot
cmd/compile: add memory-less const call and rewrite to static call
When tested, this works!
Change-Id: I1bfdb6a05c7f6e78c4dc432cde36e9adb26bdc42
---
M src/cmd/compile/internal/ssa/_gen/genericOps.go
M src/cmd/compile/internal/ssa/compile.go
M src/cmd/compile/internal/ssa/deadcode.go
M src/cmd/compile/internal/ssa/expand_calls.go
A src/cmd/compile/internal/ssa/impurify.go
M src/cmd/compile/internal/ssa/op.go
M src/cmd/compile/internal/ssa/opGen.go
M src/cmd/compile/internal/ssagen/ssa.go
8 files changed, 396 insertions(+), 16 deletions(-)
To view, visit change 533265. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Attention is currently required from: David Chase, Martin Möhrmann, Matthew Dempsky.
4 comments:
File src/cmd/compile/internal/ssa/compile.go:
Patch Set #2, Line 475:
impurifyimpurify sounds to me like it means "make a call impure". We want something more like "remove the pure part" to make it a regular call, not an "impure" one. Maybe depurify? unpurify?
File src/cmd/compile/internal/ssa/impurify.go:
Patch Set #2, Line 36:
// TODO optimize later if any of this is a performance problem.Any chance we could reuse tighten.go:memState here? I suspect not, as we also need to be adding memory modifications, but I thought I'd ask.
Patch Set #2, Line 297:
// If we've not continued the loop, that means a new memoryThis is a lot of mechanism to reintroduce memory arguments, and especially memory return values. Any chance we could just let these calls survive memoryless all the way to the end? Or do we need memory just for argument marshaling & return for rare cases (lots of args, non-regabi architectures)?
File src/cmd/compile/internal/ssagen/ssa.go:
Patch Set #2, Line 5510:
!impureI'd argue for inverting the sense of this flag. Call it "pure" so there isn't a double-negative here.
To view, visit change 533265. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: David Chase, Martin Möhrmann, Matthew Dempsky.
David Chase uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by David Chase, TryBot-Result-1 by Gopher Robot
cmd/compile: add memory-less const call and rewrite to static call
When tested, this works!
Change-Id: I1bfdb6a05c7f6e78c4dc432cde36e9adb26bdc42
---
M src/cmd/compile/internal/ssa/_gen/genericOps.go
M src/cmd/compile/internal/ssa/compile.go
M src/cmd/compile/internal/ssa/deadcode.go
M src/cmd/compile/internal/ssa/expand_calls.go
M src/cmd/compile/internal/ssa/func.go
M src/cmd/compile/internal/ssa/op.go
M src/cmd/compile/internal/ssa/opGen.go
A src/cmd/compile/internal/ssa/remove_pure.go
M src/cmd/compile/internal/ssagen/ssa.go
9 files changed, 403 insertions(+), 20 deletions(-)
To view, visit change 533265. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: Keith Randall, Martin Möhrmann, Matthew Dempsky.
4 comments:
File src/cmd/compile/internal/ssa/compile.go:
Patch Set #2, Line 475:
impurify
impurify sounds to me like it means "make a call impure". […]
It's a Dr. Strangelove reference, perhaps not the best name ( https://www.youtube.com/watch?v=gNJ4Q9jqL8A ).
Maybe just "remove pure calls"?
File src/cmd/compile/internal/ssa/impurify.go:
Patch Set #2, Line 36:
// TODO optimize later if any of this is a performance problem.
Any chance we could reuse tighten. […]
the added memory writes are what cause all the hair, sadly.
Patch Set #2, Line 297:
// If we've not continued the loop, that means a new memory
This is a lot of mechanism to reintroduce memory arguments, and especially memory return values. […]
I'm worried about getting the details of such a change right; the advantage here is that the change is limited in scope. I was thinking that I should record whether a function contained any const/pure calls and skip the rewrite if that is the case.
File src/cmd/compile/internal/ssagen/ssa.go:
Patch Set #2, Line 5510:
!impure
I'd argue for inverting the sense of this flag. […]
Acknowledged
To view, visit change 533265. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Attention is currently required from: David Chase, Martin Möhrmann, Matthew Dempsky.
9 comments:
File src/cmd/compile/internal/ssa/compile.go:
Patch Set #2, Line 475:
impurify
It's a Dr. Strangelove reference, perhaps not the best name ( https://www.youtube. […]
Great movie. The phrase that I remember is "purity of essence".
File src/cmd/compile/internal/ssa/remove_pure.go:
Patch Set #3, Line 24:
what is the Preds[0] memPresumably for blocks with a memPhi, this is irrelevant.
In other words, if memPhi[i] != nil, then firstMems[i] == memPhi[i].Args[0] ?
(Nothing to do here really, just trying to understand.)Patch Set #3, Line 26:
visited := make([]bool, f.NumBlocks())You can use f.Cache.allocBoolSlice and defer the free. For all these slice allocations in this section.
Patch Set #3, Line 35:
var lastMem *ValueMove this into the loop (to line 43)
Patch Set #3, Line 42:
anyChanged = falseYou're resetting anyChanged here, which doesn't make sense to me. If the last block processed has no change, then the following loop will never execute?
Patch Set #3, Line 119:
dependents[a.ID] = append(dependents[a.ID], v)This feels like it is going to be a lot of allocation. Likely one alloc for every Value in the function, as most values get used in the block in which they appear so they will eventually have a non-nil entry in dependents.
See how I did something similar in schedule.go, using a single prealloc'd slice using Edges + sorting + binary search.
Patch Set #3, Line 208:
// Do this rewrite after updating dependsOn/zeroDepsAll this mechanism for these 10 lines.
A possibly simplifying proposal. Define this function:
func explicitFinalMemory(f *Func, needExplicit func(b *Block) bool) {
}The resulting function has, for the blocks in which needExplicit returns true, a last memory which is an OpCopy and is used nowhere else in the block.
Given that, your change becomes a pretty simple use of explicitFinalMemory followed by a purely local block-by-block rewrite.
I'm not totally sold on my idea but I think some separation-of-concerns effort is needed here to make this understandable.
Patch Set #3, Line 234:
// Perhaps we could be clever-er about propagating change, but first, need to see if it is useful.At this point lastMems entries are all non-nil?
More generally, my brain needs to understand when these various arrays contain "what we know so far" vs. "the actual truth". I suspect that most are the former, and that makes it hard to reason about. The comment at their declaration is somewhat of a lie as it implies the latter, but really it's just aspirational about what they will contain when we reach the end of the function. What is the mid-algorithm invariant that's actually true, all the time?
Patch Set #3, Line 309:
v.SetArg(len(v.Args)-1, lastMem)This doesn't really make any sense to me. You're clobbering all the memory arguments in the whole block to lastMem?
Or are these blocks with no memory-generating ops at all? But then the l==1, lastMems[b.ID].Block == b case above needs to continue, right?
To view, visit change 533265. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
David Chase uploaded new patchset![]( "Open in Gerrit")
- Legacy-TryBots-Pass: TryBot-Result+1 by Gopher Robot, Run-TryBot+1 by David Chase
Related details
- David Chase
- Martin Möhrmann
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Chase (Gerrit)
David Chase uploaded new patchset![]( "Open in Gerrit")
- Martin Möhrmann
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |