cmd/compile: investigate test/fixedbugs/issue20250.go with GOEXPERIMENT=unified #54402

mdempsky · 2022-08-11T17:15:13Z

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

go/test/fixedbugs/issue20250.go

Line 19 in f469f20

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.

The text was updated successfully, but these errors were encountered:

mdempsky · 2022-08-11T17:32:40Z

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:

go/src/internal/buildcfg/exp.go

Lines 59 to 70 in dfbecc0

    
           // 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 · 2022-08-11T17:57:56Z

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

cuonglm · 2022-08-11T17:59:15Z

@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 · 2022-08-11T18:07:27Z

@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 · 2022-08-11T19:03:59Z

@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 · 2022-08-11T20:00:26Z

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?

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 · 2022-08-12T01:06:44Z

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 · 2023-01-10T23:12:15Z

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 · 2023-01-10T23:39:26Z

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

zikaeroh · 2023-01-22T02:00:57Z

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

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 added this to the Go1.20 milestone Aug 11, 2022

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 11, 2022

mknyszek assigned mdempsky Aug 17, 2022

mdempsky modified the milestones: Go1.20, Go1.21 Jan 10, 2023

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

gopherbot closed this as completed in ffbd194 Jan 24, 2023

golang locked and limited conversation to collaborators Jan 24, 2024

gopherbot added the FrozenDueToAge label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: investigate test/fixedbugs/issue20250.go with GOEXPERIMENT=unified #54402

cmd/compile: investigate test/fixedbugs/issue20250.go with GOEXPERIMENT=unified #54402

mdempsky commented Aug 11, 2022 •

edited

mdempsky commented Aug 11, 2022

gopherbot commented Aug 11, 2022

cuonglm commented Aug 11, 2022 •

edited

mdempsky commented Aug 11, 2022

cuonglm commented Aug 11, 2022 •

edited

mdempsky commented Aug 11, 2022

cuonglm commented Aug 12, 2022

mdempsky commented Jan 10, 2023

gopherbot commented Jan 10, 2023

zikaeroh commented Jan 22, 2023

cmd/compile: investigate test/fixedbugs/issue20250.go with GOEXPERIMENT=unified #54402

cmd/compile: investigate test/fixedbugs/issue20250.go with GOEXPERIMENT=unified #54402

Comments

mdempsky commented Aug 11, 2022 • edited

mdempsky commented Aug 11, 2022

gopherbot commented Aug 11, 2022

cuonglm commented Aug 11, 2022 • edited

mdempsky commented Aug 11, 2022

cuonglm commented Aug 11, 2022 • edited

mdempsky commented Aug 11, 2022

cuonglm commented Aug 12, 2022

mdempsky commented Jan 10, 2023

gopherbot commented Jan 10, 2023

zikaeroh commented Jan 22, 2023

mdempsky commented Aug 11, 2022 •

edited

cuonglm commented Aug 11, 2022 •

edited

cuonglm commented Aug 11, 2022 •

edited