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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: static allocation should work with constant propagation and inlining #66671

Open
dsnet opened this issue Apr 3, 2024 · 2 comments
Open

cmd/compile: static allocation should work with constant propagation and inlining #66671

dsnet opened this issue Apr 3, 2024 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@dsnet
Copy link
Member

dsnet commented Apr 3, 2024

Go version

go1.22.0

Output of go env in your module/workspace:

GOOS=linux
GOARCH=amd64

What did you do?

Run the following benchmark:

package main

import "testing"

type Option interface{ isOption() }

type maxWindowSize uint64

func (maxWindowSize) isOption() {}

func MaxWindowSize(n uint64) Option { return maxWindowSize(n) }

var opt Option

func BenchmarkFunc(b *testing.B) {
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		opt = MaxWindowSize(1024)
	}
}

func BenchmarkDirect(b *testing.B) {
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		opt = maxWindowSize(1024)
	}
}

What did you see happen?

I see:

BenchmarkFunc-24      	109294134	        11.11 ns/op	       8 B/op	       1 allocs/op
BenchmarkDirect-24    	1000000000	         0.4708 ns/op	       0 B/op	       0 allocs/op

What did you expect to see?

I expect to see:

BenchmarkFunc-24      	1000000000	         0.4708 ns/op	       0 B/op	       0 allocs/op
BenchmarkDirect-24    	1000000000	         0.4708 ns/op	       0 B/op	       0 allocs/op

The MaxWindowSize function is inlineable, so I would expect it to be identical to declaring a maxWindowSize literal directly.

This seems to be an odd interaction between static allocations, constant propagation, and inlining. I'm not sure if the title properly reflects what's going on.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 3, 2024
@dsnet dsnet mentioned this issue Apr 3, 2024
Merged
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 4, 2024
@mknyszek mknyszek added this to the Unplanned milestone Apr 10, 2024
@thanm
Copy link
Contributor

thanm commented Apr 15, 2024

Thanks for the report.

I think what's happening in the "direct" case is that this code here is kicking in:

https://go.googlesource.com/go/+/45b641ce15159e29fa4494b837493042d1e10384/src/cmd/compile/internal/walk/convert.go#168

whereas in the func case, the op in question is an auto (not a read-only stmp). It's possible to imagine adding a call to ir.StaticValue here to see if we can do the propagation, bit it would come at the cost of some compile time (ir.StaticValue has to walk the entire func). Might be worth prototyping though.

@thanm
Copy link
Contributor

thanm commented Apr 15, 2024

Might be worth prototyping though

I gave this a try and it looks like this in and of itself doesn't get us over the line. The difficulty is that the code sequence being generated by the inliner when it assigns "n" doesn't look like an initializing assignment (e.g. it has the form x = y as opposed to x := y) which is throwing off ir.StaticValue. This will have to wait for some slightly more industrial-strength new version of ir.StaticValue and/or constant propagation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Go Compiler / Runtime
Status: No status
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@mknyszek @dmitshur @dsnet @gopherbot @thanm