cmd/compile: loads combining regression #18946
Labels
Milestone
Comments
|
For ref: 1.8rc3 vs tip |
|
Well, there's also a test: but probably the pattern matching being performed at the ASM level is not so strict, and matches also the new slower generated code. /cc @randall77 who added the test :) |
|
Bisect to find commit that broke it? |
|
Bisected to https://go-review.googlesource.com/33632 |
|
And the test was in fact failing, but it isn't run in -short mode, so no one noticed. See #17472. |
TocarIP
referenced
this issue
Feb 7, 2017
CSE opportunities were being missed for commutative ops. We used to order the args of commutative ops (by arg ID) once at the start of CSE. But that may not be enough. i1 = (Load ptr mem) i2 = (Load ptr mem) x1 = (Add i1 j) x2 = (Add i2 j) Equivalent commutative ops x1 and x2 may not get their args ordered in the same way because because at the start of CSE, we don't know that the i values will be CSEd. If x1 and x2 get opposite orders we won't CSE them. Instead, (re)order the args of commutative operations by their equivalence class IDs each time we partition an equivalence class. Change-Id: Ic609fa83b85299782a5e85bf93dc6023fccf4b0c Reviewed-on: https://go-review.googlesource.com/33632 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Todd Neal <todd@tneal.org>
|
Thanks for @TocarIP this was also found here: 6317f92#commitcomment-20779055 |
|
CL https://golang.org/cl/36911 mentions this issue. |
|
@randall77 thanks for your b548eee fix. It indeed fixes original problem in this issue, but not all TestAssembly failures are gone. I'm not sure whether it is 100% related here but anyway: looks like load-combining rules without bswap are not getting triggerred. I beg you pardon in advance if this is not related to regression in 6317f92. |
|
This are s390x related. Sorry for the noise. |
|
CL https://golang.org/cl/36881 mentions this issue. |
|
The s390x failures are #19059 |
gopherbot
pushed a commit
that referenced
this issue
Feb 13, 2017
It is not always obvious from the first glance when looking at
TestAssembly failure in which context the code was generated. For
example x86 and x86-64 are similar, and those of us who do not work with
assembly every day can even take s390x version as something similar to x86.
So when something fails lets print the whole test context - this
includes os and arch which were previously missing. An example failure:
before:
--- FAIL: TestAssembly (40.48s)
asm_test.go:46: expected: MOVWZ \(.*\),
go:
import "encoding/binary"
func f(b []byte) uint32 {
return binary.LittleEndian.Uint32(b)
}
asm:"".f t=1 size=160 args=0x20 locals=0x0
...
after:
--- FAIL: TestAssembly (40.43s)
asm_test.go:46: linux/s390x: expected: MOVWZ \(.*\),
go:
import "encoding/binary"
func f(b []byte) uint32 {
return binary.LittleEndian.Uint32(b)
}
asm:"".f t=1 size=160 args=0x20 locals=0x0
Motivated-by: #18946#issuecomment-279491071
Change-Id: I61089ceec05da7a165718a7d69dec4227dd0e993
Reviewed-on: https://go-review.googlesource.com/36881
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The following code:
generates a single load on go1.7.3 and on go1.8rc3:
but it doesn't on tip
See: #14267
The text was updated successfully, but these errors were encountered: