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: s390x cannot merge conditional bool load into comparison #19227

Closed
mundaym opened this issue Feb 21, 2017 · 6 comments
Closed

cmd/compile: s390x cannot merge conditional bool load into comparison #19227

mundaym opened this issue Feb 21, 2017 · 6 comments
Labels
FrozenDueToAge

Comments

@mundaym
Copy link
Member

mundaym commented Feb 21, 2017

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

go version go1.8 linux/s390x

What did you do?

$ cat loop.go

package main

var x, y int

func main() {
        for i := 0; i < x; i++ {
                y = i
        }
}

$ GOSSAFUNC=main go run loop.go

What did you expect to see?

One comparison, like 1.8rc3:

   	00000 (/tmp/loop.go:5)	TEXT	"".main(SB)
   	00001 (/tmp/loop.go:5)	FUNCDATA	$0, "".gcargs·0(SB)
   	00002 (/tmp/loop.go:5)	FUNCDATA	$1, "".gclocals·1(SB)
v18	00003 (/tmp/loop.go:6)	MOVD	$0, R0
v8	00004 (/tmp/loop.go:6)	MOVD	"".x(SB), R1
v2	00005 (/tmp/loop.go:6)	CMP	R0, R1
b2	00006 (/tmp/loop.go:6)	BGE	10
v14	00007 (/tmp/loop.go:7)	MOVD	R0, "".y(SB)
v17	00008 (/tmp/loop.go:6)	ADD	$1, R0, R0
b4	00009 (/tmp/loop.go:6)	JMP	4
b5	00010 (/tmp/loop.go:9)	RET
   	00011 (<unknown line number>)	END

What did you see instead?

Conditional move not optimized away:

   	00000 (/tmp/loop.go:5)	TEXT	"".main(SB)
   	00001 (/tmp/loop.go:5)	FUNCDATA	$0, "".gcargs·0(SB)
   	00002 (/tmp/loop.go:5)	FUNCDATA	$1, "".gclocals·1(SB)
v22	00003 (/tmp/loop.go:6)	MOVD	$0, R0
v8	00004 (/tmp/loop.go:6)	MOVD	"".x(SB), R1
v2	00005 (/tmp/loop.go:6)	CMP	R0, R1
v6	00006 (/tmp/loop.go:6)	MOVD	$0, R1
v19	00007 (/tmp/loop.go:6)	MOVD	$1, R2
v9	00008 (/tmp/loop.go:6)	MOVDLT	R2, R1
v16	00009 (/tmp/loop.go:6)	CMPW	R1, $0
b2	00010 (/tmp/loop.go:6)	BEQ	14
v14	00011 (/tmp/loop.go:7)	MOVD	R0, "".y(SB)
v17	00012 (/tmp/loop.go:6)	ADD	$1, R0, R0
b4	00013 (/tmp/loop.go:6)	JMP	4
b5	00014 (/tmp/loop.go:9)	RET
   	00015 (<unknown line number>)	END

This regression was caused by CL 36350 (758a728) and significantly reduces the performance of small loops. I have a fix for tip, backporting the fix to 1.8.1 will also require merging CL 36547.
@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 2, 2017
@mundaym
[release-branch.go1.8] cmd/compile: remove unnecessary type conversio…
865536b 
…ns on s390x

Some rules insert MOVDreg ops to ensure that type changes are kept.
If there is no type change (or the input is constant) then the MOVDreg
can be omitted, allowing further optimization.

Reduces the size of the .text section in the asm tool by ~33KB.

For #19227.

Change-Id: I0f7b40d8dbcda73bca96eb6d2bf13f9ffa88f4b6
Reviewed-on: https://go-review.googlesource.com/37535
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Mar 2, 2017
@mundaym
[release-branch.go1.8] cmd/compile: fix merging of s390x conditional …
6a712df 
…moves into branch conditions

A type conversion inserted between MOVD{LT,LE,GT,GE,EQ,NE} and CMPWconst
by CL 36256 broke the rewrite rule designed to merge the two.
This results in simple for loops (e.g. for i := 0; i < N; i++ {})
emitting two comparisons instead of one, plus a conditional move.

This CL explicitly types the input to CMPWconst so that the type conversion
can be omitted. It also adds a test to check that conditional moves aren't
emitted for loops with 'less than' conditions (i.e. i < N) on s390x.

Fixes #19227.

Change-Id: I44958eebf6c74c5819b2a9511caf3c47c20fbf45
Reviewed-on: https://go-review.googlesource.com/37536
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bill O'Farrell <billotosyr@gmail.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Feb 28, 2018
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
No milestone
Development

No branches or pull requests
2 participants
@gopherbot @mundaym