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: code motion doesn't recognize some loads #8641

Closed
randall77 opened this issue Sep 3, 2014 · 5 comments
Closed

cmd/compile: code motion doesn't recognize some loads #8641

randall77 opened this issue Sep 3, 2014 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.5

Comments

@randall77
Copy link
Contributor 

Around newproc/deferproc we decrement/increment the stack pointer by 12 bytes.  We then
do some code motion which moves instructions from outside this region to inside this
region (and vice versa?).  Instructions that are moved inside which refer to SP must
have their offsets adjusted by 12 bytes.  This is not done for some instructions.  See
https://golang.org/cl/139160043 for an example.
@rsc
Copy link
Contributor

rsc commented Sep 15, 2014

Comment 1:

I believe we stopped doing the code motion in question.

Labels changed: added release-none, removed release-go1.4.

Status changed to Accepted.

@randall77
Copy link
Contributor Author

Comment 2:

Yes, it looks like code motion of this type no longer happens.  Any idea which change
fixed this?

@rsc
Copy link
Contributor

rsc commented Sep 15, 2014

Comment 3:

The bug was seen by changing deferproc to return its result on the stack. The memory
access was the load of the result, and it was being reordered early because it happened
right after the return. That code never made it into the repo. Because we left deferproc
returning the result in a register, the code after the return does an immediate compare
of the register + conditional jump before any memory access, so the memory accesses
cannot be reordered ahead of the conditional jump.

@griesemer
Copy link
Contributor

Comment 4:

Labels changed: added repo-main.

gopherbot pushed a commit that referenced this issue Dec 23, 2014
@randall77
runtime: make stack frames fixed size by modifying goproc/deferproc.
53c5226 
Calls to goproc/deferproc used to push & pop two extra arguments,
the argument size and the function to call.  Now, we allocate space
for those arguments in the outargs section so we don't have to
modify the SP.

Defers now use the stack pointer (instead of the argument pointer)
to identify which frame they are associated with.

A followon CL might simplify funcspdelta and some of the stack
walking code.

Fixes issue #8641

Change-Id: I835ec2f42f0392c5dec7cb0fe6bba6f2aed1dad8
Reviewed-on: https://go-review.googlesource.com/1601
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc
Copy link
Contributor

rsc commented Jun 8, 2015

This is fixed by khr's change not to do funny stack adjustments around newproc and deferproc.

@rsc rsc changed the title cmd/5g: code motion doesn't recognize some loads cmd/compile: code motion doesn't recognize some loads Jun 8, 2015
@rsc rsc closed this as completed Jun 8, 2015
@mikioh mikioh modified the milestones: Go1.5, Unplanned Jun 10, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
@rsc rsc removed their assignment Jun 23, 2022
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
5 participants
@mikioh @rsc @randall77 @griesemer @gopherbot