Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: mark the source argument to the append builtin as not escaping #4576

Closed
wants to merge 1 commit into from

Conversation

eliasnaur
Copy link
Contributor

No description provided.

@eliasnaur
Copy link
Contributor Author

eliasnaur commented Nov 1, 2024

append may return the slice; does that make the "nocapture" attribute inappropriate? Edit: stdlib test failures indicate so.

@dgryski
Copy link
Member

dgryski commented Nov 1, 2024

This looks cool. I wonder how many times this optimization actually triggers?

@eliasnaur
Copy link
Contributor Author

Good question. This change is not enough for the strconv.Append* functions to allow stack allocating the destination argument. For that, I probably need an argument-escapes-to-return-value attribute/pass like big Go does. Do you have any ideas how to approach that in TinyGo?

@dgryski
Copy link
Member

dgryski commented Nov 1, 2024

I think @aykevl had some ideas on being able to rely more on llvm for escape analysis but I don't know the status of that work.

@dgryski
Copy link
Member

dgryski commented Nov 1, 2024

I can't seem to get this to trigger.

~/go/src/github.com/dgryski/bug/heapappend $ tinygo run -print-allocs=main main.go
/Users/dgryski/go/src/github.com/dgryski/bug/heapappend/main.go:7:13: object allocated on the heap: escapes at line 9
0000
1000
0100
1100
0010
1010
0110
1110
0001
1001
0101
1101
0011
1011
0111
1111
~/go/src/github.com/dgryski/bug/heapappend $ cat main.go
package main

type nyb byte

//go:noinline
func (n nyb) String() string {
	buf := make([]byte, 0, 8)
	const digs = "01"
	buf = append(buf, digs[n&1])
	n >>= 1
	buf = append(buf, digs[n&1])
	n >>= 1
	buf = append(buf, digs[n&1])
	n >>= 1
	buf = append(buf, digs[n&1])
	return string(buf)
}

func main() {
	for i := 0; i < 16; i++ {
		println(nyb(i).String())
	}
}

@eliasnaur
Copy link
Contributor Author

I taught the escape pass about LLVM's extractvalue instruction, and your program now triggers the optimization.

@dgryski
Copy link
Member

dgryski commented Nov 2, 2024

I added some logging and ran this across the test corpus. This triggered way less frequently than I was expecting.

Logging patch:

diff --git a/transform/allocs.go b/transform/allocs.go
index da4dbbb9..d0df64a2 100644
--- a/transform/allocs.go
+++ b/transform/allocs.go
@@ -8,6 +8,7 @@ package transform
 import (
        "fmt"
        "go/token"
+       "log"
        "regexp"

        "tinygo.org/x/go-llvm"
@@ -162,6 +163,11 @@ func valueEscapesAt(value llvm.Value) llvm.Value {
                        if fn := use.CalledValue(); !fn.IsAFunction().IsNil() && fn.Name() == "runtime.sliceAppend" {
                                if at := elemEscapesAt(use, 0); !at.IsNil() {
                                        return at
+                               } else {
+                                       valPos := getPosition(value)
+                                       atPos := getPosition(at)
+                                       msg := fmt.Sprintf("sliceAppend heap->stack: %v:%v -> doesn't escape triggered %v:%v", valPos.Filename, valPos.Line, atPos.Filename, atPos.Line)
+                                       log.Println(msg)
                                }
                                break
                        }

Outputs:

cloudflare/bn256/bn256_test.go:55
VividCortex/gohistogram/weightedhistogram.go:171
VividCortex/gohistogram/numerichistogram.go:141

And from make tinygo-test-fast:
2024/11/01 21:22:18 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base32/base32_test.go:805 -> doesn't escape triggered :0
2024/11/01 21:22:18 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base32/base32_test.go:783 -> doesn't escape triggered :0
2024/11/01 21:22:18 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base32/base32_test.go:766 -> doesn't escape triggered :0
2024/11/01 21:22:18 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base32/base32_test.go:739 -> doesn't escape triggered :0
2024/11/01 21:22:19 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base64/base64_test.go:337 -> doesn't escape triggered :0
2024/11/01 21:22:19 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base64/base64_test.go:321 -> doesn't escape triggered :0
2024/11/01 21:22:19 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base64/base64_test.go:305 -> doesn't escape triggered :0
2024/11/01 21:22:19 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/encoding/base64/base64_test.go:286 -> doesn't escape triggered :0

2024/11/01 21:25:15 sliceAppend heap->stack: /Users/dgryski/go/src/go.googlesource.com/go/src/strconv/atoi_test.go:657 -> doesn't escape triggered :0

@eliasnaur
Copy link
Contributor Author

eliasnaur commented Nov 2, 2024

Indeed, without inter-procedural analysis this optimization only works within functions.

I'm tempted to look into porting big Go's escape analysis pass (https://github.com/golang/go/tree/master/src/cmd/compile/internal/escape), most importantly to avoid surprises where TinyGo heap allocates while big Go doesn't. Do you see any fundamental problems in that approach?

@eliasnaur
Copy link
Contributor Author

Filed in #4579. Closing this for now.

@eliasnaur eliasnaur closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants