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.

Sign up for GitHub

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

x/tools/go/ssa: regression in "eliminate dead φ-nodes in cycles" #19622

Closed
dominikh opened this issue Mar 20, 2017 · 4 comments
Closed

x/tools/go/ssa: regression in "eliminate dead φ-nodes in cycles" #19622

dominikh opened this issue Mar 20, 2017 · 4 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@dominikh
Copy link
Member

Input file:

package pkg

func Send(arg1 int, arg2 bool, arg3 bool) {
	var distinctSpans bool
	if arg2 {
		distinctSpans = arg1 == 0
	}
	println(arg3 && distinctSpans)
}

ssadump on 219e654~1 (last working commit):

# Name: pkg.Send
# Package: pkg
# Location: /tmp/foo.go:3:6
func Send(arg1 int, arg2 bool, arg3 bool):
0:                                                                entry P:0 S:2
	; var distinctSpans bool @ 4:6 is false:bool
	; var arg2 bool @ 5:5 is arg2
	if arg2 goto 1 else 2
1:                                                              if.then P:1 S:1
	; var arg1 int @ 6:19 is arg1
	t0 = arg1 == 0:int                                                 bool
	; *ast.BinaryExpr @ 6:19 is t0
	; var distinctSpans bool @ 6:3 is t0
	jump 2
2:                                                              if.done P:2 S:2
	t1 = phi [0: false:bool, 1: t0] #distinctSpans                     bool
	; var arg3 bool @ 8:10 is arg3
	if arg3 goto 3 else 4
3:                                                            binop.rhs P:1 S:1
	; var distinctSpans bool @ 8:18 is t1
	jump 4
4:                                                           binop.done P:2 S:0
	t2 = phi [2: false:bool, 3: t1] #&&                                bool
	; *ast.BinaryExpr @ 8:10 is t2
	t3 = println(t2)                                                     ()
	; *ast.CallExpr @ 8:2 is t3
	return

ssadump on 219e654 (first broken commit):

# Name: pkg.Send
# Package: pkg
# Location: /tmp/foo.go:3:6
func Send(arg1 int, arg2 bool, arg3 bool):
0:                                                                entry P:0 S:2
	; var distinctSpans bool @ 4:6 is false:bool
	; var arg2 bool @ 5:5 is arg2
	if arg2 goto 1 else 2
1:                                                              if.then P:1 S:1
	; var arg1 int @ 6:19 is arg1
	t0 = arg1 == 0:int                                                 bool
	; *ast.BinaryExpr @ 6:19 is t0
	; var distinctSpans bool @ 6:3 is t0
	jump 2
2:                                                              if.done P:2 S:2
	; var arg3 bool @ 8:10 is arg3
	if arg3 goto 3 else 4
3:                                                            binop.rhs P:1 S:1
	; var distinctSpans bool @ 8:18 is t1000
	jump 4
4:                                                           binop.done P:2 S:0
	t1 = phi [2: false:bool, 3: t1000] #&&                             bool
	; *ast.BinaryExpr @ 8:10 is t1
	t2 = println(t1)                                                     ()
	; *ast.CallExpr @ 8:2 is t2
	return

Problem:

t0 - the result of the assignment to distinctSpans - isn't being used, block 2 is missing a phi node, and the phi node t1 refers to an ominous t1000.

/cc @alandonovan
@dominikh dominikh mentioned this issue Mar 20, 2017
Closed
@bradfitz bradfitz added this to the Unreleased milestone Mar 21, 2017
pcc added a commit to pcc/llvm-project-old that referenced this issue Jun 5, 2017
@pcc
more stuff works but golang/go#19622
f151cc6
@dominikh
Copy link
Member Author

This is still an issue, and kind of makes go/ssa unusable for a lot of its users. Is there a way to get this on the radar of the people who actually understand the code?

@alandonovan
Copy link
Contributor

The problem seems to be that the reachability part of φ-elimination considers only "new phis" and ignores φ-nodes already present due to && and || operators. I'll prepare a fix. Sorry for the delay.

@alandonovan
Copy link
Contributor

Minimal testcase:

func f(a bool) bool {
	c := false
	if true {
		c = true
	}
	return true && c
}

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 15, 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
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @dominikh @alandonovan @gopherbot