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.

Sign up for GitHub

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: remove GOEXPERIMENT=nounified #57410

Closed
mdempsky opened this issue Dec 20, 2022 · 19 comments
Closed

cmd/compile: remove GOEXPERIMENT=nounified #57410

mdempsky opened this issue Dec 20, 2022 · 19 comments
Assignees
@mdempsky
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@mdempsky
Copy link
Member

Tracking issue for removing GOEXPERIMENT=nounified for Go 1.21.
@mdempsky mdempsky added NeedsFix The path to resolution is known, but the work has not been done. compiler/runtime Issues related to the Go compiler and/or runtime. labels Dec 20, 2022
@mdempsky mdempsky added this to the Go1.21 milestone Dec 20, 2022
@mdempsky mdempsky self-assigned this Dec 20, 2022
@gopherbot
Copy link

Change https://go.dev/cl/458624 mentions this issue: cmd/compile/internal/types: remove more unused features

@gopherbot
Copy link

Change https://go.dev/cl/458623 mentions this issue: cmd/compile/internal/types: remove NewSignature's tparams parameter

@gopherbot
Copy link

Change https://go.dev/cl/458617 mentions this issue: cmd/compile: remove -d=typecheckinl flag

@gopherbot
Copy link

Change https://go.dev/cl/458622 mentions this issue: cmd/compile/internal/types: remove Type.Pkg

@gopherbot
Copy link

Change https://go.dev/cl/458619 mentions this issue: cmd/compile/internal/noder: stop creating TUNION types

@gopherbot
Copy link

Change https://go.dev/cl/458618 mentions this issue: cmd/compile: change some unreachable code paths into Fatalf

@gopherbot
Copy link

Change https://go.dev/cl/458575 mentions this issue: cmd/compile: revert package typecheck part of CL 422914

@gopherbot
Copy link

Change https://go.dev/cl/458620 mentions this issue: cmd/compile: remove GOEXPERIMENT=nounified frontend

@gopherbot
Copy link

Change https://go.dev/cl/458616 mentions this issue: cmd/compile/internal/pkginit: remove dependency on typecheck.Resolve

@gopherbot
Copy link

Change https://go.dev/cl/458621 mentions this issue: cmd/compile/internal/types: remove TTYPEPARAM and TUNION types

@gopherbot
Copy link

Change https://go.dev/cl/458615 mentions this issue: cmd: remove GOEXPERIMENT=nounified knob

gopherbot pushed a commit that referenced this issue Jan 25, 2023
@mdempsky
cmd/compile: revert package typecheck part of CL 422914
a896219 
This code path is unreachable anyway, and it adds new uses of
Type.Pkg, which shouldn't be used anymore.

Mark Type.Pkg as deprecated while here.

Updates #57410.

Change-Id: I1eec1c8ed99207d58d0ba0c44822bbad29dc64f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/458575
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Jan 25, 2023
@mdempsky
cmd: remove GOEXPERIMENT=nounified knob
4f467f1 
This CL removes the GOEXPERIMENT=nounified knob, and any conditional
statements that depend on that knob. Further CLs to remove unreachable
code follow this one.

Updates #57410.

Change-Id: I39c147e1a83601c73f8316a001705778fee64a91
Reviewed-on: https://go-review.googlesource.com/c/go/+/458615
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky @gopherbot
cmd/compile/internal/pkginit: remove dependency on typecheck.Resolve
6d4101e 
The use of typecheck.Resolve was previously necessary to interoperate
with the non-unified frontend, because it hooked into iimport. It's no
longer necessary with unified IR, where we can just lookup the
".inittask" symbol and access Def directly.

Updates #57410.

Change-Id: I73bdfd53f65988ececd2b777743cd8b591a6db48
Reviewed-on: https://go-review.googlesource.com/c/go/+/458616
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky @gopherbot
cmd/compile: remove -d=typecheckinl flag
532e34d 
This flag forced the compiler to eagerly type check all available
inline function bodies, which presumably was useful in the early days
of implementing inlining support. However, it shouldn't have any
significance with the unified frontend, since the same code paths are
used for constructing normal function bodies as for inlining.

Updates #57410.

Change-Id: I6842cf86bcd0fbf22ac336f2fc0b7b8fe14bccca
Reviewed-on: https://go-review.googlesource.com/c/go/+/458617
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky @gopherbot
cmd/compile: change some unreachable code paths into Fatalf
de9bffb 
Now that GOEXPERIMENT=nounified is removed, we can assume InlineCall
and HaveInlineBody will always be overridden with the unified
frontend's implementations. Similarly, we can assume expandDecl will
never be called.

This CL changes the code paths into Fatalfs, so subsequent CLs can
remove all the unreachable code.

Updates #57410.

Change-Id: I2a0c3edb32916c30dd63c4dce4f1bd6f18e07468
Reviewed-on: https://go-review.googlesource.com/c/go/+/458618
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky
cmd/compile/internal/noder: stop creating TUNION types
a7de684 
In the types1 universe under the unified frontend, we never need to
worry about type parameter constraints, so we only see pure
interfaces. However, we might still see interfaces that contain union
types, because of interfaces like "interface{ any | int }" (equivalent
to just "any").

We can handle these without needing to actually represent type unions
within types1 by simply mapping any union to "any".

Updates #57410.

Change-Id: I5e4efcf0339edbb01f4035c54fb6fb1f9ddc0c65
Reviewed-on: https://go-review.googlesource.com/c/go/+/458619
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky
cmd/compile: remove GOEXPERIMENT=nounified frontend
da97613 
This CL removes most of the code specific to the nounified
frontend. To simplify review, it's a strict remove-only CL.

Updates #57410.

Change-Id: Ic3196570aa4286618c1d5e7fd0d0e6601a18195b
Reviewed-on: https://go-review.googlesource.com/c/go/+/458620
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky
cmd/compile/internal/types: remove TTYPEPARAM and TUNION types
f5eb0a7 
These were used by the nounified frontend for representing
uninstantiated generic types; however, the unified frontend only needs
types1 to represent instantiated types.

Updates #57410.

Change-Id: Iac417fbf2b86f4e08bd7fdd26ae8ed17395ce833
Reviewed-on: https://go-review.googlesource.com/c/go/+/458621
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jan 26, 2023
@mdempsky
cmd/compile/internal/types: remove unneeded functionality
f0b1563 
This CL removes a handful of features that were only needed for the
pre-unified frontends.

In particular, Type.Pkg was a hack for iexport so that
go/types.Var.Pkg could be precisely populated for struct fields and
signature parameters by gcimporter, but it's no longer necessary with
the unified export data format because we now write export data
directly from types2-supplied type descriptors.

Several other features (e.g., OrigType, implicit interfaces, type
parameters on signatures) are no longer relevant to the unified
frontend, because it only uses types1 to represent instantiated
generic types.

Updates #57410.

Change-Id: I84fd1da5e0b65d2ab91d244a7bb593821ee916e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/458622
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/463998 mentions this issue: test: enable inlining tests for functions with local type

@gopherbot
Copy link

Change https://go.dev/cl/463997 mentions this issue: cmd/compile: cleanup atomic.Pointer[T] inline test

gopherbot pushed a commit that referenced this issue Jan 31, 2023
@cuonglm @gopherbot
cmd/compile: cleanup atomic.Pointer[T] inline test
3e1478e 
Updates #57410

Change-Id: I9be38e20c6b83d14f7785049a66de77ac7ecdf15
Reviewed-on: https://go-review.googlesource.com/c/go/+/463997
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Jan 31, 2023
@cuonglm @gopherbot
test: enable inlining tests for functions with local type
ac7efcb 
Updates #57410

Change-Id: Ibe1f5523a4635d2b844b9a5db94514e07eb0bc0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/463998
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@liggitt
Copy link
Contributor

liggitt commented Feb 10, 2023 •
edited

FYI, we're seeing regressions on ARM builds in go1.20 that bisected to 833367e where the unified experiment was enabled by default (#58425 (comment))

Should the experiment remain until we're confident regressions in the unified build on go1.20 are resolved? If I understand correctly, go1.20 (released 10 days ago) is the first time the unified build was used by default, which means it has had very little time to be adopted

@mdempsky
Copy link
Member Author

The nounified frontend used in the 1.18 and 1.19 releases was the first to support generics, but we've experienced a lot of subtle correctness issues with it, which became untenable to fix. E.g., for 1.18, we decided to stop backporting generics fixes, because every time we attempted to fix one issue, we introduced at least one new one.

The unified frontend has been in use internally within Google for months. The compiler team decided as a whole to commit to the unified frontend for 1.20 and forward. We talked about whether it made sense to keep the code around longer, but we have a lot of other work planned for 1.21 (e.g., overhauling how inlining works, for better PGO support), which was hindered by the technical burden of maintaining two frontends.

We did expect hiccups in the transition though, which is why GOEXPERIMENT=nounified is still available and supported in the 1.20 release branch. And admittedly #58339 is a bit embarrassing, but that will be fixed in 1.20.1. In 1.20, it can be worked around with -gcflags=all=-d=inlstaticinit=0.

Thanks for your patience while we address the reported regressions.

@mdempsky
Copy link
Member Author

Closing this issue because the nounified frontend has been removed at tip. There's still more cleanup work to do, but that can happen without a tracking issue.

@randall77
Copy link
Contributor

@liggitt I don't think the "relocation truncated" error is particularly related to the unified frontend. That's just the change that happened to reorder functions such that the branch distance was exceeded.

When I compile your repro from #58425 successfully with 1.20, it generates a binary whose text size is 66MB, when the branch (call) distance on arm32 is +/- 32 MB. I'm kind of surprised it ever worked before - you probably just got lucky using the old frontend.

Not that there couldn't be reasons why the unified frontend exacerbates the situation, but once your program is >32MB of text it's just a matter of time before some benign change causes relocation errors. (Until #58425 is fixed, probably by adding trampolines or something.)

@liggitt
Copy link
Contributor

liggitt commented Feb 10, 2023 •
edited

When I compile your repro from #58425 successfully with 1.20, it generates a binary whose text size is 66MB, when the branch (call) distance on arm32 is +/- 32 MB. I'm kind of surprised it ever worked before - you probably just got lucky using the old frontend.

interesting... Kubernetes arm builds have been working since #17028, so 6+ years of luck was... pretty lucky

@liggitt liggitt mentioned this issue Feb 11, 2023
Closed
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@cuonglm @johanbrandhorst
cmd/compile: cleanup atomic.Pointer[T] inline test
c6e7509 
Updates golang#57410

Change-Id: I9be38e20c6b83d14f7785049a66de77ac7ecdf15
Reviewed-on: https://go-review.googlesource.com/c/go/+/463997
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@cuonglm @johanbrandhorst
test: enable inlining tests for functions with local type
7e23c91 
Updates golang#57410

Change-Id: Ibe1f5523a4635d2b844b9a5db94514e07eb0bc0f
Reviewed-on: https://go-review.googlesource.com/c/go/+/463998
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/493995 mentions this issue: cmd/compile: update README.md for unified IR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
4 participants
@mdempsky @liggitt @randall77 @gopherbot