Skip to content

cmd/compile: fix Unified IR known test failures in test/run.go #53058

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
Closed
mdempsky opened this issue May 24, 2022 · 19 comments
Closed

cmd/compile: fix Unified IR known test failures in test/run.go #53058

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

Comments

@mdempsky
Copy link
Contributor

There are still a handful of known failures for Unified IR in test/run.go:

go/test/run.go

Line 1995 in 0a30cf9

var unifiedFailures = setOf(

The tests where diagnostics spelling has changed are fine.

But the tests for linkname errors and channel/array type size issues need to be fixed. Off hand I'm not sure why the existing typecheck code isn't just doing the right thing for Unified IR, but I expect it should be easy to massage into working now. (There's no -G=0 mode anymore, so it's fine to move the existing error logic around, if that's the easiest solution.)

/cc @cuonglm @dr2chase
@mdempsky mdempsky added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 24, 2022
@mdempsky mdempsky added this to the Go1.20 milestone May 24, 2022
@cuonglm
Copy link
Member

cuonglm commented May 25, 2022

The fix for channel/array type size is trivial, I have it in my local stack for a long time. Do you want me to send CL now or wait until we have dev.unified branch?

@mdempsky
Copy link
Contributor Author

Oh great. I think now is fine, since they're bug fixes and should be small.

FYI, I'm out of office until next Tuesday though. David or Keith may be able to review though.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/408594 mentions this issue: cmd/compile: fix unified IR don't report type size too large error

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/409034 mentions this issue: cmd/compile: restore Unified IR linkname pragma diagnostic

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410341 mentions this issue: [dev.unified] cmd/compile: restore Unified IR linkname pragma diagnostic

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410340 mentions this issue: [dev.unified] cmd/compile: fix unified IR don't report type size too large error

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410345 mentions this issue: [dev.unified] cmd/compile: fix unified IR don't report type size too large error

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410346 mentions this issue: [dev.unified] cmd/compile: restore Unified IR linkname pragma diagnostic

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410574 mentions this issue: [dev.unified] cmd/compile: export/import implicit attribute for conversion exprs

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410575 mentions this issue: test: enable test issue42284.go in Unified IR

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410595 mentions this issue: [dev.unified] test: enable test issue42284.go in Unified IR

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410594 mentions this issue: [dev.unified] cmd/compile: export/import implicit attribute for conversion exprs

gopherbot pushed a commit that referenced this issue Jun 6, 2022
@cuonglm
[dev.unified] cmd/compile: export/import implicit attribute for conve…
46ddf08 
…rsion exprs

So they can be formatted more presicely, and make it easier in the
transition to Unified IR.

Updates #53058

Change-Id: I8b5a46db05a2e2822289458995b8653f0a3ffbbe
Reviewed-on: https://go-review.googlesource.com/c/go/+/410594
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410794 mentions this issue: [dev.unified] cmd/compile: set base.Pos when process assignDef in Unified IR

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410795 mentions this issue: [dev.unified] test: relax issue7921.go diagnostic message

gopherbot pushed a commit that referenced this issue Jun 8, 2022
@cuonglm
[dev.unified] cmd/compile: restore Unified IR linkname pragma diagnostic
e7ef585 
CL 333109 restore the diagnostic for irgen, now it's safe to restore for
Unified IR, too.

Updates #53058

Change-Id: I467902c0e9fa451aaa78cf0813231f14d9d7a3a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/410346
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
gopherbot pushed a commit that referenced this issue Jun 9, 2022
@cuonglm
[dev.unified] cmd/compile: fix unified IR don't report type size too …
d6df086 
…large error

For error reported during type size calculation, base.Pos needs to be
set, otherwise, the compiler will treat them as the same error and only
report once. Old typechecker and irgen all set base.Pos before
processing types, this CL do the same thing for unified IR.

Updates #53058

Change-Id: I686984ffe4aca3e8b14d2103018c8d3c7d71fb02
Reviewed-on: https://go-review.googlesource.com/c/go/+/410345
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
gopherbot pushed a commit that referenced this issue Jun 9, 2022
@cuonglm
[dev.unified] cmd/compile: set base.Pos when process assignDef in Uni…
c50c6bb 
…fied IR

CL 410343 changes Unified IR to visit LHS before RHS/X in assign/for
statement. Thus, it needs to set base.Pos before processing assignee
expression, so invalid type can be reported with correct position.

Updates #53058

Change-Id: Ic9f60cbf35c8bd71cb391e806396572c37811af7
Reviewed-on: https://go-review.googlesource.com/c/go/+/410794
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.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 Jun 9, 2022
@cuonglm
[dev.unified] test: relax issue7921.go diagnostic message
1a6c96b 
For constants literal, iimport/iexport read/write them as basic literal
nodes. So they are printed in diagnostic message as Go syntax. So "foo"
will be reported as string("foo").

Unified IR read/write the raw expression as string value, and when
printed in diagnostic, the string value is written out exactly as-is, so
"foo" will be written as "foo".

Thus, this CL relax the test in issue7921.go to match the string value only.

Updates #53058

Change-Id: I6fcf4fdcfc4b3be91cb53b081c48bd57186d8f35
Reviewed-on: https://go-review.googlesource.com/c/go/+/410795
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/411935 mentions this issue: [dev.unified] test: split inline.go into unified and non-unified tests

gopherbot pushed a commit that referenced this issue Jun 15, 2022
@cuonglm @gopherbot
[dev.unified] test: extract different inline test between unified and…
8a9485c 
… non-unified

Unified IR records the inline nodes position right at the position of
the inline call, while the old inliner always records at the position of
the original nodes.

We want to keep non-unified working up through go 1.20, thus this CL
extract the inline test case that is different in Unified IR and the old
inliner.

Updates #53058

Change-Id: I14b0ee99fe797d34f27cfec068982790c64ac263
Reviewed-on: https://go-review.googlesource.com/c/go/+/411935
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@mknyszek mknyszek moved this to Triage Backlog in Go Compiler / Runtime Jul 15, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@cuonglm @jproberts
[dev.unified] cmd/compile: export/import implicit attribute for conve…
b0522df 
…rsion exprs

So they can be formatted more presicely, and make it easier in the
transition to Unified IR.

Updates golang#53058

Change-Id: I8b5a46db05a2e2822289458995b8653f0a3ffbbe
Reviewed-on: https://go-review.googlesource.com/c/go/+/410594
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@cuonglm @jproberts
[dev.unified] cmd/compile: restore Unified IR linkname pragma diagnostic
82727f3 
CL 333109 restore the diagnostic for irgen, now it's safe to restore for
Unified IR, too.

Updates golang#53058

Change-Id: I467902c0e9fa451aaa78cf0813231f14d9d7a3a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/410346
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@cuonglm @jproberts
[dev.unified] cmd/compile: fix unified IR don't report type size too …
e756544 
…large error

For error reported during type size calculation, base.Pos needs to be
set, otherwise, the compiler will treat them as the same error and only
report once. Old typechecker and irgen all set base.Pos before
processing types, this CL do the same thing for unified IR.

Updates golang#53058

Change-Id: I686984ffe4aca3e8b14d2103018c8d3c7d71fb02
Reviewed-on: https://go-review.googlesource.com/c/go/+/410345
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@cuonglm @jproberts
[dev.unified] cmd/compile: set base.Pos when process assignDef in Uni…
1faebc5 
…fied IR

CL 410343 changes Unified IR to visit LHS before RHS/X in assign/for
statement. Thus, it needs to set base.Pos before processing assignee
expression, so invalid type can be reported with correct position.

Updates golang#53058

Change-Id: Ic9f60cbf35c8bd71cb391e806396572c37811af7
Reviewed-on: https://go-review.googlesource.com/c/go/+/410794
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@cuonglm @jproberts
[dev.unified] test: relax issue7921.go diagnostic message
8d9a02c 
For constants literal, iimport/iexport read/write them as basic literal
nodes. So they are printed in diagnostic message as Go syntax. So "foo"
will be reported as string("foo").

Unified IR read/write the raw expression as string value, and when
printed in diagnostic, the string value is written out exactly as-is, so
"foo" will be written as "foo".

Thus, this CL relax the test in issue7921.go to match the string value only.

Updates golang#53058

Change-Id: I6fcf4fdcfc4b3be91cb53b081c48bd57186d8f35
Reviewed-on: https://go-review.googlesource.com/c/go/+/410795
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@cuonglm @jproberts
[dev.unified] test: extract different inline test between unified and…
24bb51c 
… non-unified

Unified IR records the inline nodes position right at the position of
the inline call, while the old inliner always records at the position of
the original nodes.

We want to keep non-unified working up through go 1.20, thus this CL
extract the inline test case that is different in Unified IR and the old
inliner.

Updates golang#53058

Change-Id: I14b0ee99fe797d34f27cfec068982790c64ac263
Reviewed-on: https://go-review.googlesource.com/c/go/+/411935
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/437217 mentions this issue: test: split escape4.go for unified and nounified

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/437216 mentions this issue: test: relax closure name matching in closure3.go

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/437215 mentions this issue: test: enable issue47631.go for Unified IR

gopherbot pushed a commit that referenced this issue Sep 30, 2022
@cuonglm @gopherbot
test: enable issue47631.go for Unified IR
76c1a50 
Updates #53058

Change-Id: Ieaa500bea11f26f9a039196592bea67405bdf0ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/437215
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Oct 1, 2022
@cuonglm @gopherbot
test: relax closure name matching in closure3.go
0cbe304 
The mismatch between Unified IR and the old frontend is not about how
they number the closures, but how they name them. For nested closure,
the old frontend use the immediate function which contains the closure
as the outer function, while Unified IR uses the outer most function as
the outer for all closures.

That said, what important is matching the number of closures, not their
name prefix. So this CL relax the test to match both "main.func1.func2"
and "main.func1.2" to satisfy both Unified IR and the old frontend.

Updates #53058

Change-Id: I66ed816d1968aa68dd3089a4ea5850ba30afd75b
Reviewed-on: https://go-review.googlesource.com/c/go/+/437216
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Repository owner moved this from Triage Backlog to Done in Go Compiler / Runtime Oct 1, 2022
@mdempsky
Copy link
Contributor Author

mdempsky commented Oct 3, 2022

Thanks @cuonglm !

@golang golang locked and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
3 participants
@mdempsky @cuonglm @gopherbot