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: DCE before inlining does not remove dead `if`s #29189

Open
CAFxX opened this Issue Dec 12, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@CAFxX
Copy link
Contributor

CAFxX commented Dec 12, 2018

DCE performed before inlining seems to eliminate the body of ifs where the condition can be proven to always evaluate to false, but fails to eliminate the if statement and the condition themselves. So, for example, the following code:

// code before if 
if condition_known_to_be_false {
  // code inside if
}
// code after if

that should be DCEd (before inlining) to:

// code before if
// (if statement has been completely removed)
// code after if

but currently DCE transforms it to:

// code before if
if condition_known_to_be_false { /* empty body */ }
// code after if

Found by going over the output of GO_GCFLAGS=-m ./make.bash (the following is just a sample, see #8421 (comment) for details):

src/math/bits/bits.go:283:6: can inline Len as: func(uint) int { if UintSize == 32 {  }; return Len64(uint64(x)) }
src/runtime/cgocall.go:176:14: inlining call to dolockOSThread func() { if GOARCH == "wasm" {  }; _g_ := getg(); _g_.m.lockedg.set(_g_); _g_.lockedm.set(_g_.m) }
src/runtime/stack.go:1277:24: inlining call to stackmapdata func(*stackmap, int32) bitvector { if stackDebug > 0 {  }; return bitvector literal }
src/runtime/malloc.go:1105:6: can inline nextSample as: func() int32 { if GOOS == "plan9" {  }; return fastexprand(MemProfileRate) }
src/go/token/position.go:452:15: inlining call to sync.(*RWMutex).RLock method(*sync.RWMutex) func() { if bool(false) {  }; if atomic.AddInt32(&sync.rw.readerCount, int32(1)) < int32(0) { sync.runtime_SemacquireMutex(&sync.rw.readerSem, bool(false)) }; if bool(false) {  } }
src/runtime/type.go:269:20: inlining call to reflectOffsUnlock func() { if raceenabled {  }; unlock(&reflectOffs.lock) }

This is a problem because part of the inlining budget will be consumed by the vestigial ifs. If DCE before inlining completely removed those ifs more functions would likely become candidate for inlining.

Likely related to #23521 and possibly to #29095.

@CAFxX CAFxX changed the title DCE DCE before inlining does not remove dead `if`s Dec 12, 2018

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Dec 12, 2018

Are you positive that the inlining call to messages show you the nodes as they are being inlined? For example, perhaps it's showing you the original source code minus comments.

I haven't actually checked; I'm simply pointing out that this might be an issue with the -m messages and not the DCE or inline passes.

@josharian

This comment has been minimized.

Copy link
Contributor

josharian commented Dec 12, 2018

On my phone but the condition is probably being printed from n.Orig. But there are still extraneous OIF and OLITERAL (“if” and “true”) nodes there.

@josharian josharian added this to the Go1.13 milestone Dec 12, 2018

@ALTree ALTree changed the title DCE before inlining does not remove dead `if`s cmd/compile: DCE before inlining does not remove dead `if`s Dec 12, 2018

@CAFxX

This comment has been minimized.

Copy link
Contributor

CAFxX commented Dec 13, 2018

@mvdan I don't have complete certainty, but for sure those if bodies are not empty, see e.g.

go/src/runtime/malloc.go

Lines 1105 to 1114 in 944a9c7

func nextSample() int32 {
if GOOS == "plan9" {
// Plan 9 doesn't support floating point in note handler.
if g := getg(); g == g.m.gsignal {
return nextSampleNoFP()
}
}
return fastexprand(MemProfileRate)
}

src/runtime/malloc.go:1105:6: can inline nextSample as: func() int32 { if GOOS == "plan9" {  }; return fastexprand(MemProfileRate) }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment