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: investigate test/fixedbugs/issue20250.go with GOEXPERIMENT=unified #54402

Closed
mdempsky opened this issue Aug 11, 2022 · 10 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Aug 11, 2022

test/fixedbugs/issue20250.go contains this ERROR expectation:

func() { // ERROR "live at entry to f.func1: a &e"

This works fine currently on all trybots. It also works fine currently with GOEXPERIMENT=unified on most trybots.

But with GOEXPERIMENT=unified and the linux-arm and js-wasm trybots, we instead report:

live at entry to f.func1: &e a

on that line. Note: &e a instead of a &e.

Semantically, this doesn't matter. But it's still odd.

Notably, unified IR is generating identical IR for linux-amd64, linux-arm, and js-wasm:

diff -u <(GOEXPERIMENT=unified GOOS=linux GOARCH=amd64 go tool compile -l -W=2 issue20250.go) <(GOEXPERIMENT=unified GOOS=linux GOARCH=arm go tool compile -l -W=2 issue20250.go)
diff -u <(GOEXPERIMENT=unified GOOS=linux GOARCH=amd64 go tool compile -l -W=2 issue20250.go) <(GOEXPERIMENT=unified GOOS=js GOARCH=wasm go tool compile -l -W=2 issue20250.go)

so it's odd linux-arm and js-wasm are different.

Compiling for all targets listed by go tool dist list, it looks like it's dependent upon GOARCH:

  • Team a &e: amd64, arm64, ppc64, ppc64le, riscv64
  • Team &e a: 386, arm, loong64, mips64, mips64le, s390x, wasm

I thought maybe this is regabi vs no-regabi architectures, but adding noregabi to GOEXPERIMENT only causes riscv64 to switch sides. The distinction between (e.g.,) amd64 and arm remains.

I plan to just relax the test expectations to allow either a &e or &e a, because I don't think it's a real issue and its blocking enabling GOEXPERIMENT=unified by default. But filing an issue to remember to look into this further into the dev cycle.

@mdempsky mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 11, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Aug 11, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 11, 2022
@mdempsky
Copy link
Contributor Author

I thought maybe this is regabi vs no-regabi architectures, but adding noregabi to GOEXPERIMENT only causes riscv64 to switch sides. The distinction between (e.g.,) amd64 and arm remains.

Ohh, riscv64 is a regabi-supported architecture, but {amd64, arm64, ppc64, ppc64le} are regabi-always architectures:

// regabiSupported is set to true on platforms where register ABI is
// supported and enabled by default.
// regabiAlwaysOn is set to true on platforms where register ABI is
// always on.
var regabiSupported, regabiAlwaysOn bool
switch goarch {
case "amd64", "arm64", "ppc64le", "ppc64":
regabiAlwaysOn = true
regabiSupported = true
case "riscv64":
regabiSupported = true
}

So that explains why GOEXPERIMENT=noregabi only affects riscv64.

It's still a bit puzzling that the specific combination unified && !regabi changes the order, but it at least supports the hypothesis that this is just a backend optimization question.

Notably, the test does involve OCONVIFACE, and unified IR now adds explicit RTTI operands to these nodes upfront. So that plausibly influences backend code generation. And there's the generated call to runtime.convT, which could be affected by regabi.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/422975 mentions this issue: test: relax fixedbugs/issue20250.go expectations

@cuonglm
Copy link
Member

cuonglm commented Aug 11, 2022

@mdempsky The backend sort the stack variables for stable stack layout. When register ABI on, &e is passed via register, so it's after a in stack layout.

See cmd/compile/internal/ssagen/pgen.go:byStackVar.

@mdempsky
Copy link
Contributor Author

@cuonglm Thanks. But why does that sorting only change the output for GOEXPERIMENT=unified? Why do we always print the variables in the same order for GOEXPERIMENT=nounified?

@cuonglm
Copy link
Member

cuonglm commented Aug 11, 2022

@cuonglm Thanks. But why does that sorting only change the output for GOEXPERIMENT=unified? Why do we always print the variables in the same order for GOEXPERIMENT=nounified?

Seems related to how we record fn.ClosureVars. For unified, we record them as [e a], but [a e] for non-unified. Thus in directClosureCall, those closure variables are populated to fn.Dcl as [&e a]/[a &e] for unified/non-unified.

@mdempsky
Copy link
Contributor Author

Oh right, because we removed unified's quirks mode and then changed the order that we visit assignment statements (9e5c968), which in turn affects the order that we capture variables.

I remembered previously being careful that unified IR generated fn.ClosureVars in the same order. But it makes sense that it would sometimes be different now, including in this test case.

Okay, that explains GOEXPERIMENT=unified's involvement here. Thanks.

The last minor question then is why GOEXPERIMENT=nounified always prints in the same order, on both regabi and noregabi architectures?

gopherbot pushed a commit that referenced this issue Aug 11, 2022
With GOEXPERIMENT=unified, the order variables are printed in "live at
entry to f.func1" is sensitive to whether regabi is enabled for some
reason. The order shouldn't matter to correctness, but it is odd.

For now, this CL just relaxes the test expectation order to unblock
enabling GOEXPERIMENT=unified by default. I've filed #54402 to
investigate further to confirm this a concern.

Updates #54402.

Change-Id: Iddfbb12c6cf7cc17b2aec8102b33761abd5f93ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/422975
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@cuonglm
Copy link
Member

cuonglm commented Aug 12, 2022

The last minor question then is why GOEXPERIMENT=nounified always prints in the same order, on both regabi and noregabi architectures?

It's a direct closure call, so it isn't marked as need context register. Thus closure variables aren't passed via registers.

@mdempsky
Copy link
Contributor Author

Okay, I understand what's going on now. Thanks @cuonglm for the investigation. Full explanation:

Since the removal of quirks mode, the unified frontend discovers captured variables in a different order. Given e = a.s, we used to visit the RHS first, but now we visit the LHS first. So the capture order has changed from [a e] to [e a]. (This is semantically harmless, because it's invisible to users.)

However, it does have the effect that f.func1's signature changed from (effectively) func f.func1(a T, e *any) to func f.func1(e *any, a T). (Again, this is invisible to users.)

Note also that e can be passed by register, but a cannot (because T contains an array of length>1).

Also, parameters that are passed by register still get stack space reserved for them, but after any parameters that are passed on the stack.

This is why the frame offsets for (a T, e *any) would always be the same:

  • On regabi architectures, a can't be passed by register, so it gets the first stack slot; and then e gets a stack reservation immediately after it.
  • On non-regabi architectures, they both get simple stack allocations, in order.

Changing the parameter signature to (e *any, a T) though means a's stack slot is sensitive to whether e is register allocated.

Finally, that all affects how byStackVar sorts the fn.Dcl list (because parameters get sorted by stack offset); which in turn is the order that they get listed in liveness diagnostics.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/461456 mentions this issue: test: remove TODO in issue20250.go

@mdempsky mdempsky modified the milestones: Go1.20, Go1.21 Jan 10, 2023
@mdempsky mdempsky added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 10, 2023
@zikaeroh
Copy link
Contributor

With CL 462898 merged, the fix could also just be to put them in the new deterministic sort order.

@golang golang locked and limited conversation to collaborators Jan 24, 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 NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants
@mdempsky @cuonglm @gopherbot @zikaeroh and others