Skip to content

DCE even in -O0#884

Merged
kripken merged 1 commit intomasterfrom
dce-O0
Jan 19, 2017
Merged

DCE even in -O0#884
kripken merged 1 commit intomasterfrom
dce-O0

Conversation

@kripken
Copy link
Member

@kripken kripken commented Jan 19, 2017

This is forced upon us as Firefox is landing a patch that rejects unreachable code. I don't fully understand what's going on in the spec process about this, but seems like binaryen and other producers have no other options but to not emit unreachable code.

This has some downsides, as discussed in #862, namely it makes -O0 a tiny bit slower. It also means -O0 now optimizes, so it might make debugging harder (and perhaps we'll need an "-O0 without DCE" mode at some point). So depending on the spec process perhaps we'll end up reverting it.

@dschuff
Copy link
Member

dschuff commented Jan 19, 2017

I have no problem with this for now; DCE is simple and cheap. I do think it would be an interesting exercise to try to avoid generating the structurally-unreachable code in the first place without relying on powerful IR. I have a feeling it might not be too bad and would likely be worth doing, and would also avoid the need for an O0 which produces invalid code (and in the event that we decide that it is valid, we can just back this change out anyway).

@kripken
Copy link
Member Author

kripken commented Jan 19, 2017

Yeah, the rule seems to be "disallow code until the end of a block, once you see a br or return etc.", so we could do that more directly in e.g. the binary writing code, pretty easily. Not sure what's better for debugging, downsides either way. For now just using the DCE pass seems simplest.

@dschuff
Copy link
Member

dschuff commented Jan 19, 2017

The other option, if we really do want to have all the code in the binary for debugging, would just be to wrap the br in a block by itself.

@kripken
Copy link
Member Author

kripken commented Jan 19, 2017

Oh right, yeah, we could work around it that way too. Simple option too, if we need it.

Landing as there are no concerns and I'm already getting reports of code failing due to this.

@kripken kripken merged commit 00f4322 into master Jan 19, 2017
@kripken kripken deleted the dce-O0 branch January 19, 2017 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants