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.

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

cmd/compile: enable PAUTO captured variables on arch != 6 #9865

Closed
dvyukov opened this issue Feb 13, 2015 · 2 comments
Closed

cmd/compile: enable PAUTO captured variables on arch != 6 #9865

dvyukov opened this issue Feb 13, 2015 · 2 comments
Labels
FrozenDueToAge
Milestone
Go1.5

Comments

@dvyukov
Copy link
Member

dvyukov commented Feb 13, 2015

The following changelist:
cmd/gc: transform closure calls to function calls
https://go-review.googlesource.com/#/c/4050/

adds an optimization to transform captured variables (PPARAMREF) to plain PAUTO variables:

cmd/gc/closure.c:transformclosure:

            if(v->byval && v->type->width <= 2*widthptr && arch.thechar == '6') {
                // If it is a small variable captured by value,
                // downgrade it to PAUTO.
                v->class = PAUTO;
                v->ullman = 1;
                xfunc->dcl = list(xfunc->dcl, v);
                body = list(body, nod(OAS, v, cv));
            } else {

However, it is enabled only for amd64 because of issues with codegen on at least 386 (have not tested arm,power,etc). The problem is that local vars created during copyout smash context.

The following code in cmd/internal/objfile/disass.go:

func (d *Disasm) lookup(addr uint64) (name string, base uint64) {
    i := sort.Search(len(d.syms), func(i int) bool {    return addr < d.syms[i].Addr })

Generates the following code on 386 with the optimization enabled:

Dump of assembler code for function cmd/internal/objfile.func·001:
   0x080cdbb0 <+0>: mov    %gs:0x0,%ecx
   0x080cdbb7 <+7>: mov    -0x8(%ecx),%ecx
   0x080cdbbd <+13>:    cmp    0x8(%ecx),%esp
   0x080cdbc0 <+16>:    ja     0x80cdbc9 <cmd/internal/objfile.func·001+25>
   0x080cdbc2 <+18>:    call   0x80876b0 <runtime.morestack>
   0x080cdbc7 <+23>:    jmp    0x80cdbb0 <cmd/internal/objfile.func·001>
   0x080cdbc9 <+25>:    sub    $0x14,%esp
   0x080cdbcc <+28>:    lea    0x4(%edx),%ebx
   0x080cdbcf <+31>:    mov    (%ebx),%eax
   0x080cdbd1 <+33>:    mov    0x4(%ebx),%edx  // HERE: copy out of 'i' smashes context
   0x080cdbd4 <+36>:    mov    %eax,0x8(%esp)
   0x080cdbd8 <+40>:    mov    %edx,0xc(%esp)
   0x080cdbdc <+44>:    mov    0xc(%edx),%ebx // HERE: try to copy out d from context which is now 0

We need to fix codegen on 386 and test other archs.

@rsc @minux @DanielMorsing @aclements @josharian
@dvyukov dvyukov added this to the Go1.5 milestone Feb 13, 2015
@rsc
Copy link
Contributor

rsc commented May 19, 2015

I agree we need to fix this for the other architectures. If we can't, we should disable it entirely. I don't want to be seen as favoring amd64 in this way.

@rsc rsc changed the title cmd/gc: enable PAUTO captured variables on arch != 6 cmd/compile: enable PAUTO captured variables on arch != 6 Jun 8, 2015
@gopherbot
Copy link

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

@rsc rsc closed this as completed in 27edd72 Jun 29, 2015
@golang golang locked and limited conversation to collaborators Jun 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
3 participants
@rsc @dvyukov @gopherbot