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

cmd/compile: gcflags="-d=ssa/check_bce" doesn't work or doesn't work well for generic functions #58139

Closed
zigo101 opened this issue Jan 30, 2023 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge generics Issue is related to generics NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@zigo101
Copy link

zigo101 commented Jan 30, 2023

What version of Go are you using (go version)?

$ go version
go version go1.20rc3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

type ByteSeq interface{ ~string | ~[]byte }

//go:noinline
func CommonPrefixes[X, Y ByteSeq](x X, y Y) (X, Y) {
	min := len(x)
	if min > len(y) {
		min = len(y)
	}
	x2 := x[:min]
	y2 := y[:len(x2)]
	for i := 0; i < len(x2); i++ {
		if x2[i] != y2[i] {
			return x2[:i], y2[:i]
		}
	}
	return x2, y2
}

var _, _ = CommonPrefixes("", []byte{})
var _, _ = CommonPrefixes("", "")
var _, _ = CommonPrefixes([]byte{}, "")
var _, _ = CommonPrefixes([]byte{}, []byte{})

func main() {}
go run -gcflags="-d=ssa/check_bce"  main.go

What did you expect to see?

./main.go:11:9: Found IsSliceInBounds
./main.go:12:9: Found IsSliceInBounds

What did you see instead?

./main.go:11:9: Found IsSliceInBounds
./main.go:12:9: Found IsSliceInBounds
./main.go:14:17: Found IsInBounds

If the all variable declaration lines are removed, then there is no bound check found messages.
If the latter 3 variable declaration lines are removed, then there will be only two bound check found messages

This makes library maintainers mistake there is no or less bound checks, except that the maintainers list all possible function instances (like the variable declaration lines).

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 30, 2023
@greatroar
Copy link

Not all inlining decisions can be made prior to instantiation. Consider:

func f[T byte | int](a *[256]int, i T) int { return a[i] }

T=byte triggers BCE, but T=int can't.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 30, 2023
@mknyszek mknyszek added this to the Backlog milestone Jan 30, 2023
@mknyszek
Copy link
Contributor

CC @golang/compiler

@mknyszek mknyszek added the generics Issue is related to generics label Jan 30, 2023
@randall77
Copy link
Contributor

I'm not sure what the right thing to do here is. Optimization decisions might be different for different instantiations. Right now we basically report the union of all the optimizations across instantiations. That's inaccurate, but I don't see an obvious accurate way to report them that isn't very verbose.

That said, this is a pretty rare situation. Usually optimizations will be the same.

The fact that we get no reports at all if you don't instantiate something is also unfortunate, but I'm not sure what the solution to that is either. We could instantiate with some default types but how would we do that? If the bound has method Foo how do we find a type that has Foo to instantiate with? Maybe there aren't any in the package being compiled?

@zigo101
Copy link
Author

zigo101 commented Feb 1, 2023

It certainly has some improvement room there.
Some bound checks are type arguments independent,
For example, no bound checks for the following function if it is never instantiated,
but all instantiations of the functions will do bound checks.

func foo[T any](s []T) {
	_ = s[0]
	_ = s[1]
	_ = s[2]
}

On the other hand, I do understand that custom generics are not free meat.
Thanks for the explanation.

Please close this issue if nothing needs to be done.

@golang golang locked and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge generics Issue is related to generics NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants