Skip to content

Simplify br_if by removing its value operand. #709

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

Closed
wants to merge 1 commit into from

Conversation

sunfishcode
Copy link
Member

This proposes removing br_if's value operand (effectively by fixing the arity to 0) to defer the question of how its return value should be handled. We'd have the flexibility to reintroduce this operand in the future.

This new PR is in place of reopening #681 which github won't let me re-open.

@kripken
Copy link
Member

kripken commented Jun 17, 2016

I think this is a good option since

  1. If we don't do this then we'd need to spend more time investigating (assuming the current data isn't convincing enough, which is fair).
  2. Functionally, we can still do a conditional br with a value, using a br inside an if.

@ghost
Copy link

ghost commented Jun 18, 2016

It's still not a great outcome as the 'arity' immediate is still there consuming space, just because a decision can not be made. Why not just propose to remove the 'arity' entirely, and if added in future then a separate opcode can just be added and we still have the more compact opcode for this high frequency case. Also the operator table has not been proposed yet, and there seems reasons to expect some objections particularly if it is a burden for a literal wasm interpreter.

@dschuff
Copy link
Member

dschuff commented Jul 7, 2016

Still catching up after vacation, but I think this is a good idea too.

@sunfishcode
Copy link
Member Author

br_if is being simplified via WebAssembly/spec#330 instead.

@sunfishcode sunfishcode deleted the simplify-br_if branch September 13, 2016 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants