-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: fix s390x load-combining rules #19059
Comments
CL https://golang.org/cl/36911 mentions this issue. |
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)? |
The test might be wrong, but if you look at the generated assembly it is all 1-byte loads. |
OK, I'll take a look. |
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 <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Josh Bleecher Snyder <[email protected]>
CL https://golang.org/cl/36843 mentions this issue. |
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.
The text was updated successfully, but these errors were encountered: