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: a phi instruction is not emitted when possible #47165

Closed
pupiles opened this issue Jul 13, 2021 · 3 comments
Closed

x/tools/go/ssa: a phi instruction is not emitted when possible #47165

pupiles opened this issue Jul 13, 2021 · 3 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unplanned

Comments

@pupiles
Copy link

pupiles commented Jul 13, 2021

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

$ go version
go version go1.16.3 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
darwin/amd64

What did you do?

the code same as

https://github.com/tmc/ssaview/blob/master/ssaview.go

when parse the code below,it should be product phi inst,but not

package main

func test11(n int) {
	if n > 1 {
		n += 2
	} else if n == 2 {
		n += 3
	} else {
		n -= 1
	}
	fmt.Println(n)
}
func main(){}

What did you expect to see?

product phi inst

What did you see instead?
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 13, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jul 13, 2021
@timothy-king timothy-king changed the title x/tools/go/ssa no phi inst x/tools/go/ssa: a phi instruction is not emitted when possible Jul 13, 2021
@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Jul 13, 2021
@timothy-king timothy-king modified the milestones: Unreleased, Unplanned Jul 13, 2021
@timothy-king
Copy link
Contributor

ssa currently emits what looks like a valid translation that uses a memory address for n instead of using phi instructions. While this is sub-optimal and it would be nice to fix, the ssa does not look incorrect.

The place to start addressing this is func lift(fn *Function) in ssa/lift.go.

This is a reasonable place for a community contribution if this is a pressing concern. Moving this to Unplanned.

@cherrymui cherrymui added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Jul 13, 2021
@dominikh
Copy link
Member

From ssaview's code:

	ssaProg := ssautil.CreateProgram(prog, ssa.NaiveForm|ssa.BuildSerially)

this is building the "naive" representation, which means the lifting phase gets skipped entirely. If we don't skip lifting, then we do get the expected phi nodes.

@pupiles
Copy link
Author

pupiles commented Jul 14, 2021

@dominikh yes

if f.Prog.mode&NaiveForm == 0 {
		// For debugging pre-state of lifting pass:
		// numberRegisters(f)
		// f.WriteTo(os.Stderr)
		lift(f)
	}

if mode&NativeForm equal 0,i will get phi node correctly
thanks a lot

@golang golang locked and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@dominikh @timothy-king @gopherbot @cherrymui @pupiles