-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
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). |
@quasilyte, go for it. The Go 1.11 tree opens soon (week?) anyway. |
Change https://golang.org/cl/91056 mentions this issue: |
This rule triggers few times during //=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... |
Change https://golang.org/cl/104555 mentions this issue: |
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>
Suppose these two functionally identical definitions of
f
:f1
can be inlined, butf2
is only inlineable ifconstX <= constY
is false.The problem with
f1
is that it will triggergo 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 forif/else
, it conflicts withgo 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:
The text was updated successfully, but these errors were encountered: