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

cmd/compile: ICE: "must have bodyReader for type..hash.[type]" #58572

Closed
mdempsky opened this issue Feb 16, 2023 · 8 comments
Closed

cmd/compile: ICE: "must have bodyReader for type..hash.[type]" #58572

mdempsky opened this issue Feb 16, 2023 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone
Go1.21

Comments

@mdempsky
Copy link
Member

We have a Google-internal package that's failing to build since 25f5d9d (https://go.googlesource.com/go/+/25f5d9d, /cc @cuonglm).

Mostly minified test case here: https://go.dev/play/p/jddWxVs4sf0?v=gotip
@mdempsky mdempsky added NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages) labels Feb 16, 2023
@mdempsky mdempsky added this to the Go1.21 milestone Feb 16, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 16, 2023
@mdempsky
Copy link
Member Author

Further minimized: https://go.dev/play/p/fkRXZrXwIfe?v=gotip

@mdempsky
Copy link
Member Author

Since CL 436961 isn't essential, I'm leaning towards reverting it for now and adding the regress test case; and then we can try that CL again but without breaking the test case.

@gopherbot
Copy link

Change https://go.dev/cl/468880 mentions this issue: test: add regress test for #58572

@gopherbot
Copy link

Change https://go.dev/cl/468879 mentions this issue: Revert "cmd/compile: use ONAME node directly from generated hash func"

gopherbot pushed a commit that referenced this issue Feb 17, 2023
@mdempsky @gopherbot
Revert "cmd/compile: use ONAME node directly from generated hash func"
95be547 
This reverts commit 25f5d9d.

Causes ICE on valid code.

Updates #58572.

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

cuonglm commented Feb 17, 2023 •
edited

@mdempsky seems that we should relax the check in bodyReaderFor to assert only in case of local functions and imported generic functions?

Previously, the type hash functions is generated during local package compilation, but they are from pseudo package type, not local package. Before 25f5d9d, we create these ONAME nodes with nil func, so they couldn't reach unifiedHaveInlineBody check. Now they do, which causes the ICE.

@mdempsky
Copy link
Member Author

@mdempsky seems that we should relax the check in bodyReaderFor to assert only in case of local functions?

I think it would be a reasonable short-term solution to exempt the synthetic hash and eq functions, but I wouldn't want to more broadly relax that assert. The inliner is able to gracefully fallback when an inline body is missing, but that's not really intended operation, and it degrades the quality of the inliner's cost heuristics.

I think a better solution would be to change inline.CanInline to recognize these functions, and mark them as non-inlineable.

Longer term, if there's a reason we do want to be able to inline these eq/hash functions, then maybe we need to integrate the code generation into the unified frontend's "synthetic" function mechanism.

@gopherbot
Copy link

Change https://go.dev/cl/469017 mentions this issue: cmd/compile: mark type eq/hash functions non-inlineable

@gopherbot
Copy link

Change https://go.dev/cl/469018 mentions this issue: Revert "Revert "cmd/compile: use ONAME node directly from generated hash func""

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
@mdempsky @johanbrandhorst
Revert "cmd/compile: use ONAME node directly from generated hash func"
e12b013 
This reverts commit 25f5d9d.

Causes ICE on valid code.

Updates golang#58572.

Change-Id: Ib276c87d9b0362bbd2a760ac2a242f82d4e20400
Reviewed-on: https://go-review.googlesource.com/c/go/+/468879
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
@mdempsky @johanbrandhorst
test: add regress test for golang#58572
bea2382 
Fixes golang#58572.

Change-Id: I75fa432afefc3e036ed9a6a9002a29d7b23105ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/468880
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Feb 28, 2023
@cuonglm @gopherbot
cmd/compile: mark type eq/hash functions non-inlineable
7fb075d 
The compiler used to generate ONAME node with nil Func for them, so the
inliner can still analyze, but could not generate inline call for them
anyway.

CL 436961 attempts to create ONAME node with non-nil Func, causing the
inliner complains about missing body reader.

This CL makes inliner recognize type eq/hash functions, and mark them as
non-inlineable. Longer term, if we do want to inline these functions, we
need to integrate the code generation into Unified IR frontend.

Updates #58572

Change-Id: Icdd4dda03711929faa3d48fe2d9886568471f0bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/469017
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Feb 28, 2023
@cuonglm @gopherbot
cmd/compile: use ONAME node directly from generated hash func
eee2697 
This reverts CL 468879

CL 469017 marked type eq/hash functions as non-inlineable, so this
change won't cause ICE anymore.

Updates #58572

Change-Id: I3e6ec9ba2217102693acd1848a0eba0886dc9fda
Reviewed-on: https://go-review.googlesource.com/c/go/+/469018
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Feb 17, 2024
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 NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Milestone
Go1.21
Development

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