-
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: redundant conditional jump #29853
Comments
Yeah, that looks bad. Digging through the SSA, looks like we're not using a consistent type for itabs. Some are Shouldn't be a hard fix. Leaving for 1.13. |
Huh. I'm seeing something different. The In case anyone wants to look at this again before I do (dunno when that might be), here's my replacement for trimmableBlock: func trimmableBlock(b *Block) bool {
if b == b.Func.Entry {
return false
}
switch b.Kind {
case BlockDefer, BlockRet, BlockRetJmp, BlockExit:
return false
case BlockPlain:
s := b.Succs[0].b
return s != b && (len(s.Preds) == 1 || emptyBlock(b))
}
if len(b.Preds) != 1 {
return false
}
p := b.Preds[0].b
if p.Kind != b.Kind || p.Control != b.Control {
// TODO: detect inverted kind?
return false
}
s0, s1 := b.Succs[0].b, b.Succs[1].b
if s0 == b || s1 == b {
return false
}
if p.Succs[0].b != b { // TODO: allow b in either position
return false
}
return true
} Things to do here include: make the code less ugly, fix a panic that this induces when generating debug info, handle inverted block kinds (i.e. NE b1 b2 === EQ b2 b1), and handle the case in which the duplicated block is not the first successor of its predecessor (which requires fixing up the calling code in cc @ysmolsky @mvdan |
For the code:
After CSE, we have the following SSA:
|
Right. But after
Then lowered CSE manages to combine the (This dump is from two passes later, to eliminate the dead values)
So all is well, except that we don't eliminate the duplicate conditional block. |
I guess I was thinking that prove would remove the duplicate block if the comparisons were redundant. |
Makes sense. Seems like there's some experimentation to do. FWIW, I can tell you that switching that particular |
Change https://golang.org/cl/229984 mentions this issue: |
Fixed by the CL stack starting at https://go-review.googlesource.com/c/go/+/599255 |
What did you do?
Hello. I was writing some code and decided to inspect the assembly instructions generated from it.
https://godbolt.org/z/zFjGRF
What did you expect to see?
I was expecting to see no redundant instructions.
What did you see instead?
There is a redundant conditional jump on line 31.
It is redundant, because the jump from line 24 to 31 happens if AX != 0 (that is, err is != nil). But on line 31, the conditional jump tests if AX == 0, which is never true.
The text was updated successfully, but these errors were encountered: