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

cmd/compile: improve const expr if/else DCE #23521

Closed
quasilyte opened this issue Jan 23, 2018 · 5 comments
Closed

cmd/compile: improve const expr if/else DCE #23521

quasilyte opened this issue Jan 23, 2018 · 5 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@quasilyte
Copy link
Contributor

quasilyte commented Jan 23, 2018 •
edited

Suppose these two functionally identical definitions of f:

func f1() T {
	if constX <= constY {
		return funcA() // cost(funcA) ~ 60
	} else {
		return funcB() // cost(funcB) ~ 60
	}
} // cost(f1) < 80

func f2() T {
	if constX <= constY {
		return funcA() // cost(funcA) ~ 60
	}
	return funcB() // cost(funcB) ~ 60
} // cost(f2) > 120 (or < 80 if condition is false)

f1 can be inlined, but f2 is only inlineable if constX <= constY is false.

The problem with f1 is that it will trigger go lint warning (if block ends with a return statement, so drop this else and outdent its block).

Example of where it strikes: if you do a const-expression branching depending on the bits.UintSize, inlining may not happen if code does not follow exact pattern. And even if you know the pattern for if/else, it conflicts with go lint.

Related to #17566.

This issue is only about pre-inlining DCE, which affects function cost.

I am going to link this issue in the upcoming CL that uses explicit else in this case to trigger dead code elimination before inlining.

Update: @josharian improved this change to also handle "else" branches,
so code like this stays inlineable as well:

func f() {
	// Suppose deadcode sections contain panics or non-leaf calls.
	if !truth {
		// deadcode (eliminated before CL).
	} else {
		return // or panic
	}
	// deadcode (with this CL)
}
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 23, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Jan 23, 2018
@quasilyte
Copy link
Contributor Author

If there is no objections, I would like to try solving this and send CL in near future (even though it's unlikely to be reviewed until 1.11).

@bradfitz
Copy link
Contributor

@quasilyte, go for it. The Go 1.11 tree opens soon (week?) anyway.

@gopherbot
Copy link

Change https://golang.org/cl/91056 mentions this issue: cmd/compile: make DCE remove nodes after terminating if

@quasilyte
Copy link
Contributor Author

This rule triggers few times during $ ./make.bash on linux/amd64.
Providing these cases for additional context.

//=trigger= runtime/lfstack_64bit.go:41:6 lfstackUnpack
if GOARCH == "amd64" {
    return (*lfnode)(unsafe.Pointer(uintptr(int64(val) >> cntBits << 3)))
}
// Everything below is removed...

//=trigger= runtime/malloc.go:401:6 sysAlloc
// If using 64-bit, our reservation is all we have.
if sys.PtrSize != 4 {
    return nil
}
// Everything below is removed...

//=trigger= path/filepath/path.go:165:6 ToSlash
//=trigger= path/filepath/path.go:175:6 FromSlash
if Separator == '/' {
    return path
}
// Everything below is removed...

//=trigger= path/filepath/symlink.go:15:6 isRoot
if runtime.GOOS != "windows" {
    return path == "/"
}
// Everything below is removed...

//=trigger= path/filepath/symlink.go:29:6 isDriveLetter
if runtime.GOOS != "windows" {
    return false
}
// Everything below is removed...

//=trigger= math/big/float.go:353:6 validate
if !debugFloat {
    // avoid performance bugs
    panic("validate called but debugFloat is not set")
}
// Everything below is removed...

//=trigger= math/big/float.go:1179:6 validateBinaryOperands
if !debugFloat {
    // avoid performance bugs
    panic("validateBinaryOperands called but debugFloat is not set")
}
// Everything below is removed...

@gopherbot
Copy link

Change https://golang.org/cl/104555 mentions this issue: math/big: remove "else" from if with block that ends with return

gopherbot pushed a commit that referenced this issue Apr 3, 2018
@bradfitz
math/big: remove "else" from if with block that ends with return
02952ad 
That "else" was needed due to gc DCE limitations.
Now it's not the case and we can avoid go lint complaints.
(See #23521 and https://golang.org/cl/91056.)

There is inlining test for bigEndianWord, so if test
is passing, no performance regression should occur.

Change-Id: Id84d63f361e5e51a52293904ff042966c83c16e9
Reviewed-on: https://go-review.googlesource.com/104555
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@CAFxX CAFxX mentioned this issue Dec 12, 2018
Closed
@golang golang locked and limited conversation to collaborators Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
3 participants
@bradfitz @quasilyte @gopherbot