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.

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/internal/obj/ppc64, cmd/link/internal/ppc64: change function alignment to 16 #18963

Closed
ceseo opened this issue Feb 6, 2017 · 3 comments
Closed

cmd/internal/obj/ppc64, cmd/link/internal/ppc64: change function alignment to 16 #18963

ceseo opened this issue Feb 6, 2017 · 3 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.9Maybe

Comments

@ceseo
Copy link
Contributor

ceseo commented Feb 6, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8rc3

What operating system and processor architecture are you using (go env)?

ppc64le, Linux

What did you do?

I noticed that cmd/internal/obj/ppc64/asm9.go and cmd/link/internal/ppc64/l.go set funcAlign to 8 bytes.

What did you expect to see?

The function alignment on Power should be 16 bytes, as stated by the User Manual: "Branches not from the last instruction of an aligned quadword and not to the first instruction of an aligned quadword cause inefficiencies in the IBuffer".

I'll submit a small change shortly to address this.
@gopherbot
Copy link

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

@laboger
Copy link
Contributor

laboger commented Feb 6, 2017

Other compilers and assemblers that target ppc64x use function alignment 16.
This change is related to #14935. We need the right function alignment first to determine if adding an alignment directive is worthwhile.

Carlos is generating some performance information on this change as requested.

@quentinmit quentinmit added this to the Go1.9Maybe milestone Feb 27, 2017
@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 27, 2017
@quentinmit quentinmit changed the title cmd/internal/obj/ppc64, cmd/link/internal/ppc64 - Change function alignment to 16 cmd/internal/obj/ppc64, cmd/link/internal/ppc64: change function alignment to 16 Feb 27, 2017
@laboger
Copy link
Contributor

laboger commented Apr 24, 2017

It is hard to find a benchmark that shows a performance improvement due to this change alone, because most golang mini benchmarks are highly dependent on loops, and if the function alignment is changed then loop alignment might change from good to bad causing a degradation.

All other compilers and assemblers for ppc64x use 16 as the function alignment. In theory it should improve performance by a few percent. It would be more important if we had a way to indicate which loops should be aligned but we currently do not.

ceseo added a commit to powertechpreview/go that referenced this issue May 11, 2017
@ceseo
cmd/internal/obj/ppc64, cmd/link/internal/ppc64: Change function alig…
ee4a2bc 
…nment to 16

The Power processor manual states that "Branches not from the last instruction
of an aligned quadword and not to the first instruction of an aligned quadword
cause inefficiencies in the IBuffer". This changes the function alignment from 8
to 16 bytes to comply with that.

Fixes golang#18963

Change-Id: Ibce9bf8302110a86c6ab05948569af9ffdfcf4bb
Reviewed-on: https://go-review.googlesource.com/36390
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Lynn Boger <laboger@linux.vnet.ibm.com>
@golang golang locked and limited conversation to collaborators May 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
4 participants
@quentinmit @ceseo @gopherbot @laboger