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

Open
mdempsky opened this Issue May 14, 2015 · 9 comments

Comments

Projects
None yet
8 participants
@mdempsky
Member

mdempsky commented May 14, 2015

This code from internal/syscall/unix/getrandom_linux.go:

package unix
import "runtime"
var randomTrap = map[string]uintptr{
        "386":     355,
        "amd64":   318,
        "arm":     384,
        "ppc64":   359,
        "ppc64le": 359,
}[runtime.GOARCH]

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.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone May 15, 2015

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 15, 2015

@randall77 randall77 self-assigned this May 15, 2015

@ianlancetaylor ianlancetaylor changed the title from cmd/internal/gc: constant indexing into map literals should be folded to cmd/compile: constant indexing into map literals should be folded Sep 3, 2015

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 4, 2015

Or people could not write code like that. Come on.

@rsc rsc modified the milestones: Unplanned, Go1.6 Nov 4, 2015

@mdempsky

This comment has been minimized.

Member

mdempsky commented Nov 4, 2015

It looks like changing the code to:

var randomTrap = func() uintptr {
    switch runtime.GOARCH {
    case "386":
        return 355
    case "amd64":
        return 318
    case "arm":
        return 384
    case "ppc64", "ppc64le":
        return 359
    default:
        return 0
    }
}()

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.)

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 4, 2015

Is this in any way a performance issue (I can't see why)? If not, what's wrong with the switch?

@mdempsky

This comment has been minimized.

Member

mdempsky commented Nov 4, 2015

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.

@gopherbot

This comment has been minimized.

gopherbot commented Nov 4, 2015

CL https://golang.org/cl/16662 mentions this issue.

@davecheney

This comment has been minimized.

Contributor

davecheney commented Nov 4, 2015

@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!

mdempsky added a commit that referenced this issue Nov 5, 2015

internal/syscall/unix: eliminate non-trivial randomTrap initializer
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>
@gopherbot

This comment has been minimized.

gopherbot commented Mar 21, 2016

CL https://golang.org/cl/20975 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 21, 2016

internal/syscall/unix: document randomTrap
Updates #10848

Change-Id: I8353100ed01cb0e8fc19225157f5709bae388612
Reviewed-on: https://go-review.googlesource.com/20975
Reviewed-by: Rob Pike <r@golang.org>
@rsc

This comment has been minimized.

Contributor

rsc commented Mar 21, 2016

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:

CL https://golang.org/cl/20975 mentions this issue.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#10848 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment