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: constant indexing into map literals should be folded #10848
Comments
/cc @randall77 |
Or people could not write code like that. Come on. |
It looks like changing the code to:
produces simpler code by taking advantage of the existing switch constant folding logic, but still needs a dynamic initializer. Alternatively, we could just create 4 extra files and use +build rules. (This is my preference if no one's opposed.) |
Is this in any way a performance issue (I can't see why)? If not, what's wrong with the switch? |
Not a performance issue. Just trying to reduce executable bloat. Side benefit of using +build rules, it will remind porters to update this file when adding new architectures. E.g., I just realized arm64 isn't using getrandom. |
CL https://golang.org/cl/16662 mentions this issue. |
@mdempsky I think this is the best approach. There are already per GOOS and GOARCH files for all permutations, and you get to make that value a constant declaration. win/win! |
While here, enable getrandom on arm64 too (using the value found in include/uapi/asm-generic/unistd.h, which seems to match up with other GOARCH=arm64 syscall numbers). Updates #10848. Change-Id: I5ab36ccf6ee8d5cc6f0e1a61d09f0da7410288b9 Reviewed-on: https://go-review.googlesource.com/16662 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
CL https://golang.org/cl/20975 mentions this issue. |
Updates #10848 Change-Id: I8353100ed01cb0e8fc19225157f5709bae388612 Reviewed-on: https://go-review.googlesource.com/20975 Reviewed-by: Rob Pike <r@golang.org>
Indeed. That map thing is a terrible idiom and should not be encouraged. On Mon, Mar 21, 2016 at 4:01 AM GopherBot notifications@github.com wrote:
|
One observation while teaching Go: I noticed a lot of people with python as their first and Go now as their second language write this. Python has no switch statement, so using map (or dict in python) seems ideomatic to them for this use case, when in fact switch is the "table to expression" ideom for those cases in Go. Any idea where might be a good place to clear that up? Adding an example in the tour comes to mind here. |
If this is the case, apart of improving the documentation, could we add this case to |
Change https://golang.org/cl/231041 mentions this issue: |
The switch statement can be statically optimized by the compiler, whereas similarly optimizing the map index expression would require additional compiler analysis to detect the map is never mutated. Updates #10848. Change-Id: I2fc70d4a34dc545677b99f218b51023c7891bbbd Reviewed-on: https://go-review.googlesource.com/c/go/+/231041 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
I don't think this rises to the level of a vet check. |
Is it in scope for a lint check? |
The switch statement can be statically optimized by the compiler, whereas similarly optimizing the map index expression would require additional compiler analysis to detect the map is never mutated. Updates golang#10848. Change-Id: I2fc70d4a34dc545677b99f218b51023c7891bbbd Reviewed-on: https://go-review.googlesource.com/c/go/+/231041 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
I don't think we have any intention of optimizing indexing of map literals like this. Users should use (Caveat: until #34381 is implemented, switch statements are limited to binary search. But it's more likely we'll fix that than optimize this use case.) |
This code from internal/syscall/unix/getrandom_linux.go:
currently compiles into non-trivial chunk of code. However, gc could instead recognize that runtime.GOARCH and all of the map literal keys are constants and that all of the value expressions are side-effect free, and fold to just the corresponding value expression.
The text was updated successfully, but these errors were encountered: