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

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

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

Comments

Projects
None yet
3 participants
@quasilyte
Copy link
Contributor

commented Jan 23, 2018

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 label Jan 23, 2018

@bradfitz bradfitz added this to the Go1.11 milestone Jan 23, 2018

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Feb 1, 2018

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

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

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 gopherbot closed this in dcaf3fb Apr 3, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Apr 3, 2018

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

math/big: remove "else" from if with block that ends with return
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>

@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.
You can’t perform that action at this time.