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: -l does not disable intra-package inlining #44585

Closed
egonelbre opened this issue Feb 24, 2021 · 6 comments
Closed

cmd/compile: -l does not disable intra-package inlining #44585

egonelbre opened this issue Feb 24, 2021 · 6 comments

Comments

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Feb 24, 2021

-l seems to have different behavior depending on whether the func is imported from another package or not. This came up in this changeset: https://go-review.googlesource.com/c/go/+/295989.

Test with -N -l failed with this version of the code:

func EncodeQuad(d []uint32, x [6]float32) { // ERROR "can inline EncodeQuad" "d does not escape"
	_ = d[:6]
	d[0] = math.Float32bits(x[0]) // ERROR "inlining call to math.Float32bits"
	d[1] = math.Float32bits(x[1]) // ERROR "inlining call to math.Float32bits"
	d[2] = math.Float32bits(x[2]) // ERROR "inlining call to math.Float32bits"
	d[3] = math.Float32bits(x[3]) // ERROR "inlining call to math.Float32bits"
	d[4] = math.Float32bits(x[4]) // ERROR "inlining call to math.Float32bits"
	d[5] = math.Float32bits(x[5]) // ERROR "inlining call to math.Float32bits"
}

It's weird that -l only disables some of the inlining.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 24, 2021

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 24, 2021

On the noopt builder, package math will have been compiled with -l, so Float32bits will have been marked as not inlinable. So I think that failure makes sense.

I'm a little less sure why the local function would fix it. Maybe because test/run.go doesn't pass GO_GCFLAGS through to the compiler when invoked for errorcheck tests? I seem to remember some inconsistency like that. And if package A imports package B, and B was compiled with -l, then even if package A is compiled without -l then functions imported from B won't be inlinable.

Loading

@egonelbre
Copy link
Contributor Author

@egonelbre egonelbre commented Feb 25, 2021

I was looking into test/run.go and it does have some special behavior for noopt.

go/test/run.go

Line 396 in 37ca84a

noOptEnv: strings.Contains(gcFlags, "-N") || strings.Contains(gcFlags, "-l"),

And some of the tests are disabled with those flags

// +build amd64,!gcflags_noopt

But, the code looks like it does pass the gc flags:

go/test/run.go

Line 1113 in 37ca84a

cmd := []string{goTool(), "run", goGcflags()}

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 25, 2021

Loading

@egonelbre
Copy link
Contributor Author

@egonelbre egonelbre commented Feb 25, 2021

Hmm, so it does look like the code does what it is written to do :D. And the explanation that math was compiled with -l does make sense.

I don't really have anything actionable that could be done here, since the tooling seems to work as intended.

Loading

@josharian
Copy link
Contributor

@josharian josharian commented Feb 25, 2021

Thanks for investigating. This makes sense now; or anyway, I understand now. :)

Loading

@josharian josharian closed this Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants