-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: make algs content-addressable #17029
Comments
I'm not convinced pointer-vs-scalar was a problem with CL 19842. Like @randall77 pointed out, we already use the AMEM algs for pointers (see algtype1). |
Pointer-vs-scalar matters if the stack gets copied. The current implementation of algs means potentially making arbitrarily deep function calls, which means that we can't mark the algs nosplit. IIRC, the options I thought were worth pursing were (1) distinguishing pointers and scalars in the naming convention and (2) only use structural naming for functions that can be marked nosplit. |
Doesn't pointer-vs-scalar only matter for stack copying if we save a pointer value in a stack slot across a function call? I don't think the hashing/equality algs do that. |
Well, we do make function calls (e.g. for cross-package equality routines). I'd be nervous about asserting that no pointers ever get spilled in the process. |
I don't think it's likely that But if that is something we're worried about, we could invent a new compiler-internal ambiguous-pointer type to use for these comparisons, and arrange for Fatalf if they end up in gclocals. E.g., by making haspointers complain. |
Yeah, as long as we detect the regression and fail if it occurs, I'd be happy. |
When we generate algs, we name them after the types they operate on. However, all that matters to the alg is the structure of the type. Equality and hashing is the same for
struct { A [2]int64}
andstruct { B [2]int64 }
. I took an initial stab at this in CL 19842 but it didn't properly handle pointers vs scalars. The CL suggested a possible binary size improvement of ~1%. This issue is a reminder to try again, probably after some alg-related cleanup.cc @cherrymui
The text was updated successfully, but these errors were encountered: