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: instruction reordering with optimizations disabled (go1.21) #58482

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

Comments

@aarzilli
Copy link
Contributor

aarzilli commented Feb 12, 2023

The function main.Foo in teststepconcurrent.go compiled with -gcflags=-N -l now produces:

teststepconcurrent.go:13	SP	0x482c80	4883ec10	SUBQ $0x10, SP
teststepconcurrent.go:13		0x482c84	48896c2408	MOVQ BP, 0x8(SP)
teststepconcurrent.go:13		0x482c89	488d6c2408	LEAQ 0x8(SP), BP
teststepconcurrent.go:15	S	0x482c8e	488d0c18	LEAQ 0(AX)(BX*1), CX
teststepconcurrent.go:13	S	0x482c92	4889442418	MOVQ AX, 0x18(SP)
teststepconcurrent.go:13		0x482c97	48895c2420	MOVQ BX, 0x20(SP)
teststepconcurrent.go:13		0x482c9c	48c7042400000000	MOVQ $0x0, 0(SP)
teststepconcurrent.go:15		0x482ca4	48890c24	MOVQ CX, 0(SP)
teststepconcurrent.go:16	S	0x482ca8	4889c8	MOVQ CX, AX
teststepconcurrent.go:16		0x482cab	488b6c2408	MOVQ 0x8(SP), BP
teststepconcurrent.go:16		0x482cb0	4883c410	ADDQ $0x10, SP
teststepconcurrent.go:16		0x482cb4	c3	RET

The block of instructions assigned to the function header (:13) after the (LEAQ) addition is unfortunate.
Before it used to be:

teststepconcurrent.go:13	SP	0x482ac0	4883ec10	SUBQ $0x10, SP
teststepconcurrent.go:13		0x482ac4	48896c2408	MOVQ BP, 0x8(SP)
teststepconcurrent.go:13		0x482ac9	488d6c2408	LEAQ 0x8(SP), BP
teststepconcurrent.go:13		0x482ace	4889442418	MOVQ AX, 0x18(SP)
teststepconcurrent.go:13		0x482ad3	48895c2420	MOVQ BX, 0x20(SP)
teststepconcurrent.go:13		0x482ad8	48c7042400000000	MOVQ $0x0, 0(SP)
teststepconcurrent.go:15	S	0x482ae0	4801d8	ADDQ BX, AX
teststepconcurrent.go:15		0x482ae3	48890424	MOVQ AX, 0(SP)
teststepconcurrent.go:16	S	0x482ae7	488b6c2408	MOVQ 0x8(SP), BP
teststepconcurrent.go:16		0x482aec	4883c410	ADDQ $0x10, SP
teststepconcurrent.go:16		0x482af0	c3	RET

This was introduced by 12befc3 (cmd/compile: improve scheduling pass). cc @randall77 @dr2chase

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 12, 2023
@prattmic
Copy link
Member

cc @golang/compiler

@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2023
@prattmic prattmic added this to the Go1.21 milestone Feb 14, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/468455 mentions this issue: cmd/compile: schedule SP earlier

@randall77 randall77 self-assigned this Feb 15, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/468615 mentions this issue: cmd/compile: ensure constant folding of pointer arithmetic remains a pointer

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/468695 mentions this issue: cmd/compile: ensure InitMem comes after Args

gopherbot pushed a commit that referenced this issue Feb 16, 2023
The debug info generation currently depends on this invariant.

A small update to CL 468455.

Update #58482

Change-Id: Ica305d360d9af04036c604b6a65b683f7cb6e212
Reviewed-on: https://go-review.googlesource.com/c/go/+/468695
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue Feb 17, 2023
…pointer

For c + nil, we want the result to still be of pointer type.

Fixes ppc64le build failure with CL 468455, in issue33724.go.

The problem in that test is that it requires a nil check to be
scheduled before the corresponding load. This normally happens fine
because we prioritize nil checks. If we have nilcheck(p) and load(p),
once p is scheduled the nil check will always go before the load.

The issue we saw in 33724 is that when p is a nil pointer, we ended up
with two different p's, an int64(0) as the argument to the nil check
and an (*Outer)(0) as the argument to the load. Those two zeroes don't
get CSEd, so if the (*Outer)(0) happens to get scheduled first, the
load can end up before the nilcheck.

Fix this by always having constant arithmetic preserve the pointerness
of the value, so that both zeroes are of type *Outer and get CSEd.

Update #58482
Update #33724

Change-Id: Ib9b8c0446f1690b574e0f3c0afb9934efbaf3513
Reviewed-on: https://go-review.googlesource.com/c/go/+/468615
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Bypass: Keith Randall <khr@golang.org>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
The actual scheduling of SP early doesn't really matter, but lots of
early spills (of arguments) depend on SP so they can't schedule until
SP does.

Fixes golang#58482

Change-Id: Ie581fba7cb173d665c11f797f39d824b1c040a2d
Reviewed-on: https://go-review.googlesource.com/c/go/+/468455
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
The debug info generation currently depends on this invariant.

A small update to CL 468455.

Update golang#58482

Change-Id: Ica305d360d9af04036c604b6a65b683f7cb6e212
Reviewed-on: https://go-review.googlesource.com/c/go/+/468695
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: David Chase <drchase@google.com>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
…pointer

For c + nil, we want the result to still be of pointer type.

Fixes ppc64le build failure with CL 468455, in issue33724.go.

The problem in that test is that it requires a nil check to be
scheduled before the corresponding load. This normally happens fine
because we prioritize nil checks. If we have nilcheck(p) and load(p),
once p is scheduled the nil check will always go before the load.

The issue we saw in 33724 is that when p is a nil pointer, we ended up
with two different p's, an int64(0) as the argument to the nil check
and an (*Outer)(0) as the argument to the load. Those two zeroes don't
get CSEd, so if the (*Outer)(0) happens to get scheduled first, the
load can end up before the nilcheck.

Fix this by always having constant arithmetic preserve the pointerness
of the value, so that both zeroes are of type *Outer and get CSEd.

Update golang#58482
Update golang#33724

Change-Id: Ib9b8c0446f1690b574e0f3c0afb9934efbaf3513
Reviewed-on: https://go-review.googlesource.com/c/go/+/468615
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Bypass: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Feb 16, 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 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

4 participants