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/compile: fix s390x load-combining rules #19059

Closed
randall77 opened this issue Feb 13, 2017 · 5 comments
Closed

cmd/compile: fix s390x load-combining rules #19059

randall77 opened this issue Feb 13, 2017 · 5 comments
Labels
FrozenDueToAge Performance
Milestone
Go1.9

Comments

@randall77
Copy link
Contributor 

func f(b []byte) uint64 {
	return binary.LittleEndian.Uint64(b)
}

CL 33632 broke a bunch of the load-combining rules.
CL 36911 fixes the issue that broke them, at least for x86. It also adds a test for s390x load combining.

Unfortunately, even with the CL 36911 fix, the s390x load-combining tests fail. I haven't really looked into it yet, hoping an s390x expert can take a look.
@randall77 randall77 added this to the Go1.9 milestone Feb 13, 2017
@gopherbot
Copy link

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

@mundaym
Copy link
Member

mundaym commented Feb 13, 2017

I had a quick look, the tests you added are little endian but the instructions you check for aren't the byte reversal instructions (MOVDBR, MOVWBR and MOVHBR). Is that the problem (s390x is big endian)?

@randall77
Copy link
Contributor Author

The test might be wrong, but if you look at the generated assembly it is all 1-byte loads.

@mundaym
Copy link
Member

mundaym commented Feb 13, 2017

OK, I'll take a look.

gopherbot pushed a commit that referenced this issue Feb 13, 2017
@randall77
cmd/compile: fix load-combining rules
b548eee 
CL 33632 reorders args of commutative ops in order to make
CSE for commutative ops more robust.  Unfortunately, that
broke the load-combining rules which depend on a certain ordering
of OR ops' arguments.

Introduce some additional rules that order OR ops' arguments
consistently so that the load-combining rules fire.

Note: there's also something else wrong with the s390x rules.
I've filed #19059 for that.

Fixes #18946

Change-Id: I0a5447196bd88a55ccee683c69a57b943a9972e1
Reviewed-on: https://go-review.googlesource.com/36911
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@randall77 randall77 mentioned this issue Feb 13, 2017
Closed
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Feb 13, 2018
@rsc rsc unassigned mundaym 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 Performance
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
3 participants
@randall77 @gopherbot @mundaym