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

x/build: new builder for cmd/compile's "unified IR" mode #46786

Closed
mdempsky opened this issue Jun 16, 2021 · 19 comments
Closed

x/build: new builder for cmd/compile's "unified IR" mode #46786

mdempsky opened this issue Jun 16, 2021 · 19 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. new-builder
Milestone
Unreleased

Comments

@mdempsky
Copy link
Member

mdempsky commented Jun 16, 2021 •
edited

I'd like to add a builder for testing the compiler's new "unified IR" mode. This has landed on dev.typeparams, and it took all of 1 commits for us to accidentally break it again.

Proposed build config (caveat: this is just copy/paste/tweaked based on some of the other compiler experiment configs; I don't feel strongly about any of these if other changes would be appropriate):

        addBuilder(BuildConfig{
                Name:     "linux-amd64-unified-ir",
                HostType: "host-linux-buster",
                Notes:    "builder with GO_GCFLAGS=-d=unified=1, see golang.org/issue/46786",
                buildsRepo: func(repo, branch, goBranch string) bool {
                        return repo == "go" && branch == "dev.typeparams"
                },
                env: []string{
                        "GO_DISABLE_OUTBOUND_NETWORK=1",
                        "GO_GCFLAGS=-d=unified=1",
                },
                GoDeps: []string{
                        "cf1ae5fc364eb7f2ee5203e4c5e30411c3cfe01f", // dev.typeparams commit that added -d=unified
                },
        })

Caveat: It's expected that the go tool dist test test:0_1 step will fail with unified IR currently. (There's a list of tests that are known to fail with the new types2 type checker, but test/run.go doesn't know -d=unified enables this too. I will fix this shortly.)
@mdempsky mdempsky added Builders x/build issues (builders, bots, dashboards) new-builder labels Jun 16, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jun 16, 2021
@mdempsky
Copy link
Member Author

I just tried GO_GCFLAGS=-d=unified=1 and it exploded a lot harder in test/run.go than I was expecting. Maybe it would be more robust as a GOEXPERIMENT instead.

@gopherbot
Copy link

Change https://golang.org/cl/328215 mentions this issue: [dev.typeparams] all: add GOEXPERIMENT=unified knob

@gopherbot
Copy link

Change https://golang.org/cl/328891 mentions this issue: dashboard: add linux-amd64-unified builder

gopherbot pushed a commit that referenced this issue Jun 17, 2021
@mdempsky
[dev.typeparams] all: add GOEXPERIMENT=unified knob
804ecc2 
Setting `-gcflags=all=-d=unified` works for normal builds/tests, but
seems to have trouble with the test/run.go regress tests. So add a
GOEXPERIMENT knob to allow another way to turn on unified IR
construction, which plays better with all.bash.

While here, update two existing test expectations that currently fail
during GOEXPERIMENT=unified ./all.bash:

1. misc/cgo/errors/testdata/err2.go is testing column positions, and
types2 gets one case slightly better, and another case slightly
worse. For now, the test case is updated to accept both.

2. fixedbugs/issue42284.go is added to the list of known failures,
because it fails for unified IR. (It's an escape analysis test, and
escape analysis is working as expected; but unified is formatting an
imported constant value differently than the test's regexp expects.)

Updates #46786.

Change-Id: I40a4a70fa1b85ac87fcc85a43687f5d81e011ec0
Reviewed-on: https://go-review.googlesource.com/c/go/+/328215
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 17, 2021
@cagedmantis
Copy link
Contributor

/cc @golang/release

@dmitshur
Copy link
Contributor

From looking at the CL, I understand this issue needs to be updated to mention TryBots (that is, pre-submit builders too, not just post-submit ones). If this will be helpful for development, adding the builder makes sense, so moving to NeedsFix.

@mdempsky Do you expect it'll be helpful to have the builder indefinitely, or is there a point at which it'll stop being needed (because sufficient coverage becomes provided elsewhere)? Or is it not possible to predict this time (that's fine; I just wanted to ask)?

@dmitshur dmitshur 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 Jun 17, 2021
@mdempsky
Copy link
Member Author

@dmitshur Sorry, I saw @cagedmantis 's CR+2 before seeing your questions here.

@mdempsky mdempsky reopened this Jun 17, 2021
@mdempsky
Copy link
Member Author

Do you expect it'll be helpful to have the builder indefinitely, or is there a point at which it'll stop being needed (because sufficient coverage becomes provided elsewhere)?

This builder will only be needed for as long as we have the unified experiment. I'm hoping to make that the default for Go 1.18 (and feel reasonably confident that will happen), but there's a chance it doesn't make it until Go 1.19. I expect we'll be able to remove the experiment again shortly after it's released and enabled as default.

Or is it not possible to predict this time (that's fine; I just wanted to ask)?

I definitely don't see it living indefinitely. I think we'll need it for less than a year, and possibly much less than that depending on how generics progress.

@dmitshur
Copy link
Contributor

Thanks. Having that information written down here will be helpful in the future. I think it's okay to close the issue since the builder is added and my question is answered, unless you're aware of something more to do here.

@mdempsky
Copy link
Member Author

@dmitshur Cool, sounds good. Thanks, closing again then.

@mdempsky
Copy link
Member Author

One more QQ: I assume this has to wait for the x/build code to be re-deployed? What's the timeframe like for that? (No rush, just interested in making sure things are working.)

@cagedmantis
Copy link
Contributor

I can deploy the builder as soon as there are no active remote buildlets in use. I can notify you once it's deployed. I expect it would be a little later on today.

@mdempsky
Copy link
Member Author

@cagedmantis Later today sounds great, thanks.

@cagedmantis
Copy link
Contributor

@mdempsky The coordinator has been deployed.

@mdempsky
Copy link
Member Author

mdempsky commented Jun 17, 2021 •
edited

@cagedmantis Thanks, the trybot looks like it's working great (after submitting golang.org/cl/329269 to fix a compiler bug w/ unified mode).

One thing I notice though: the "unified" column has appeared on the dev.typeparams branch page (except), but it isn't showing any results yet: https://build.golang.org/?repo=&branch=dev.typeparams

Did I mess up the builder config? Something wrong with the GoDeps commit ID maybe?

@mdempsky mdempsky reopened this Jun 17, 2021
@cagedmantis
Copy link
Contributor

@mdempsky I will take a look at this in a bit.

@cagedmantis
Copy link
Contributor

@mdempsky I'm still troubleshooting this. I'm pulling in some other team members to take a look at this point.

@cagedmantis cagedmantis mentioned this issue Jun 21, 2021
Open
@gopherbot
Copy link

Change https://golang.org/cl/329929 mentions this issue: cmd/coordinator: enable post submit builder for linux-amd64-unified

@gopherbot
Copy link

Change https://golang.org/cl/341931 mentions this issue: dashboard: enable linux-amd64-unified on master

gopherbot pushed a commit to golang/build that referenced this issue Aug 13, 2021
@mdempsky
dashboard: enable linux-amd64-unified on master
4f8007b 
dev.typeparams has been merged to master, so continue testing that
GOEXPERIMENT=unified works there too.

Updates golang/go#46786.

Change-Id: Iea048ef081f4c40a5b1c516a7c0d68178fd0b196
Reviewed-on: https://go-review.googlesource.com/c/build/+/341931
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@cagedmantis
Copy link
Contributor

The coordinator has been deployed. It includes the changes in CL 341931.

@DmitriyMV DmitriyMV mentioned this issue Dec 16, 2021
Closed
@mdempsky mdempsky mentioned this issue Feb 28, 2022
Closed
@golang golang locked and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. new-builder
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mdempsky @cagedmantis @dmitshur @gopherbot