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

Jump to bottom

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
Labels
FrozenDueToAge

Comments

@egonelbre
Copy link
Contributor

egonelbre commented Feb 24, 2021 •
edited by josharian

-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

cc @mdempsky

@mdempsky
Copy link
Member

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.

@egonelbre
Copy link
Contributor Author

egonelbre commented Feb 25, 2021 •
edited

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

go/test/checkbce.go

Line 1 in 37ca84a

// +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()}

@mdempsky
Copy link
Member

mdempsky commented Feb 25, 2021 via email

@egonelbre
Copy link
Contributor Author

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.

@josharian
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mdempsky @josharian @egonelbre @gopherbot