-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
x/build: new builder for cmd/compile's "unified IR" mode #46786
Comments
I just tried |
Change https://golang.org/cl/328215 mentions this issue: |
Change https://golang.org/cl/328891 mentions this issue: |
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>
/cc @golang/release |
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 Sorry, I saw @cagedmantis 's CR+2 before seeing your questions here. |
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.
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. |
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. |
@dmitshur Cool, sounds good. Thanks, closing again then. |
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.) |
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. |
@cagedmantis Later today sounds great, thanks. |
@mdempsky The coordinator has been deployed. |
@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 |
@mdempsky I will take a look at this in a bit. |
@mdempsky I'm still troubleshooting this. I'm pulling in some other team members to take a look at this point. |
Change https://golang.org/cl/329929 mentions this issue: |
Change https://golang.org/cl/341931 mentions this issue: |
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>
The coordinator has been deployed. It includes the changes in CL 341931. |
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):
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.)The text was updated successfully, but these errors were encountered: