Skip to content
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

Simplify br_if by removing its value operand. #681

Closed
wants to merge 1 commit into from

Conversation

sunfishcode
Copy link
Member

#677 is the latest in a long and complex discussion (WebAssembly/spec#179, WebAssembly/spec#180, WebAssembly/spec#271, and others) about how to impose high-level typing rules on an assembly-style branch instruction. I and several other people actively involved in WebAssembly's design have been confused about what the rules are, which is not auspicious for its broader adventures.

The current state, where br_if has a value operand, but no return value, may satisfy certain properties relevant to the high-level typing system, but it is awkward from an assembly perspective, which is the perspective that motivated the instruction in the first place. It's essentially creating an optimized path for the case where a value is live on the branch-taken side of the branch but not the fallthrough side.

The bigger picture is that I don't think we've had the appropriate chance to figure out how we really want this instruction to work. I propose that br_if's arity be limited to 0 in the MVP. We can change it to permit 1 in the future when we work out how we want it to work.

Addressing the concern of code size, the numbers being quoted for block return values in general are in the neighborhood of 1%. I don't know the specific fraction of that which involves br_if, but even in the worst case, I claim this is rounding error when compared to the broader concern of designing a clean language.

@rossberg
Copy link
Member

rossberg commented May 2, 2016

I don't quite follow. The design discussion you cite is not specific to br_if, so what is gained by removing the argument just there?

Also, this question will be pretty much moot when we eliminate implicit dropping, which probably is the way to go.

@sunfishcode
Copy link
Member Author

If you think it would simplify things even more, then perhaps we should also remove the value operand from all branches, in the MVP. The data in #667 shows that it wouldn't have a big impact on overall code size. Given time to properly design how this feature should work, and data demonstrating that it has significant value, we can reintroduce it.

Also, I don't see how implicit dropping affects this. The br_if problem I described above would still apply, as I understand it.

@rossberg
Copy link
Member

rossberg commented May 2, 2016

Not simplify "more", but at all. ;) FWIW, I mentioned that very option on #677 a couple of days ago, where Alon replied "I certainly hope not".

That other br_if "problem" you mention could as well be solved by adding a return value, like you proposed a while ago. On the other hand, the discussion about removing implicit dropping also involved making more operators void, including sets and stores. That means that this "problematic" case would actually become the rule. Are you opposed to that as well?

@lukewagner
Copy link
Member

What's the problem with having br_if return its value operand (if branch not taken)? Is this a problem that would be easier to solve with the explicit-drop rules? That seems to me to be the most regular design; I had to double-check check.ml to see that wasn't already the case...

@ghost
Copy link

ghost commented May 2, 2016

Even if there were some small utility for returning the value argument from br_if, the producer will likely have many live-out definitions so without multiple-value support this only helps for one of these values and the rest need to use the local variable support anyway. This was part of the rationale for the expressionless proposal #669, that multiple-value expression support will never make it into wasm, and even if it did it's still limited to single use cases or would require destructuring back to local variables anyway.

What's the harm in just making the current br_if a special br0_if opcode and leaving people to add new opcodes in future if they really want to explore returning a value? Then the immediate arities can be removed. The zero value case is so very common that developers will be using the operator table to create it anyway, and runtimes will likely have specialized handling for these variations in the operator table, so why not just add opcodes for it now. Another way to look at these is special drop/break operators, which developers would be creating in the operator table too as these are the common case.

With block and br still returning values, br_if can still support diamond patterns returning values, but br has the same type-system issues, so why not just have these also return no values.

For the predictor proposed to optimize the expressionless encoding, if a 'value' is written in order before the condition is written to a local variable then this value will be predicted to be the next value used, as if it were returned from br_if, but the cost of a prediction miss is now lower.

@lukewagner
Copy link
Member

Hah, actually looking again, it appears we've actually already implemented br_if-returning-it's-value in SM (it's the obvious/symmetric thing to do if branches have values).

@ghost
Copy link

ghost commented May 3, 2016

@lukewagner Re 'obvious/symmetric thing to do', it's like branching in a stack machine in which case the target and fall-through both have the same number of values on the stack, and accepting value(s) to the break operators gives something similar but on the implicit expression value stack. But in wasm without multiple-value expressions we can not have two or more values on this stack, and for example have the target and fall-through push another value and then make a call consuming these. The predictor for the expressionless encoding assumes a stack-like allocation so could achieve good prediction hits for such cases, and if there's a miss it is a small cost as operators accept immediate local variables, and it can be easily decoded into uses of local variables rather than a stack machine.

@sunfishcode
Copy link
Member Author

sunfishcode commented May 3, 2016

The data in #677#667 seems likely to end up in the neighborhood of 1% overall code size. And for that, we've ventured into uncharted territory here -- no other language or bytecode or ISA I know of has conditional branches with value operands as wasm does.

Making br_if return its operand sounds like an improvement. However, if it interacts with explicit dropping, then perhaps there could be a different problem. The safe thing would be to defer it until we can learn more, as it would seem the cost is low.

@ghost
Copy link

ghost commented May 3, 2016

@sunfishcode Re 'no other language or bytecode or ISA I know of has conditional branches with value operands as wasm does': viewing the wasm expressions as a stack with restricted use, then CIL is a bytecode example that allows a conditional branch with values on the stack which could encode a wasm br_if passing values to the target and the pass-through.

@ghost
Copy link

ghost commented May 3, 2016

Just to give a concrete example:

(block $l (i32.add (br_if $l <v3> <cond>) <c>))

could be expressed in a stack machine as:

<v3>
<cond>
br_if $l
<c>
add
$l:

@kripken
Copy link
Member

kripken commented May 3, 2016

we've ventured into uncharted territory here -- no other language or bytecode or ISA I know of has conditional branches with value operands as wasm does.

I think you can look at branches with values as phis. More specifically, if A branches either to B with a value, or otherwise continues to C without a value, then at B we would have a phi (consisting of all the other paths branching into B), and C would not have any phis.

In that perspective, I don't think there's anything odd about a conditional br with a value. The value "belongs" to the phi, which belongs to the target, not to the brancher. And everything branching to the target will provide a value, and so we have proper symmetry.

Instead of SSA form and phis, though, we have non-SSA locals and branches. But the parallel is there. And there is precedent even without the parallel, in Swift, which has SSA form but no phis, instead it has basic block arguments - branches are like calls in that they provide values that are received on the other side. A br with a value is pretty much that. And that's the same with or without a condition on the br.

@ghost
Copy link

ghost commented May 3, 2016

@kripken The Swift blocks sound similar to https://github.com/stoklund/cretonne/blob/master/docs/langref.rst But this has separate SSA variables, plus the locals (local stack), and the wasm expression results seem to match the SSA variables. It also claims to be able to convert uses of the locals into SSA form which is what the expressionless wasm encoding depends on and thus has no need for the SSA variables. The question is what is the most compact encoding to get the job done here, and with a low complexity for the runtime, and while not being too complicated to decode to something readable?

@rossberg
Copy link
Member

rossberg commented May 3, 2016

On 2 May 2016 at 21:01, Luke Wagner notifications@github.com wrote:

What's the problem with having br_if return its value operand (if branch
not taken)? Is this a problem that would be easier to solve with the
explicit-drop rules? That seems to me to be the most regular design; I had
to double-check check.ml to see that wasn't already the case...

That was #237. We deferred it because of technicalities with break typing
that needed resolving first. But with #271 landed, that has happened, so
it's time to revisit it.

However, isn't this feature obsolete if we want to go down the explicit
dropping road?

@sunfishcode
Copy link
Member Author

@kripken I agree that phis and wasm's br_if have a relationship, but there are enough conceptual differences that treating them as the same is confusing to me. A consumer can construct SSA form in a simple manner from br_if, though it may not be minimal. A producer with SSA form can't always translate a phi into a br_if operand.

@JSStats That's a good example, and if br_if is made to return its value (in the AST sense) or leave 1 value on the stack (in a stack machine sense), it would be close to that.

@rossberg-chromium My understanding of the explicit dropping idea is that it's fairly orthogonal to this issue. Having more values in more places might require more explicit drops, but are there other interactions?

@rossberg
Copy link
Member

rossberg commented May 3, 2016

@sunfishcode, well, our assumption so far was that explicit dropping is only feasible if we reduce the number of "spurious" non-void operators, particularly those whose result isn't providing any information. That includes sets and stores, but clearly, br_if is in the same class (you'd probably need a drop around the majority of br_ifs with argument if you make them have a result).

@lukewagner
Copy link
Member

@rossberg-chromium You won't need to explicitly drop br_if with arity 0 which would, it seems, be the 99% majority case. Otherwise, if we're in this 1% case where there actually is a live value across the branch, it stands to reason that it could be used by both successors of the branch (otherwise, you'd expect it to be in the branch in which it was used). So this seems like call: if the drop-requiring case were more common, we'd add a drop form, but it doesn't seem to be so we can hold off.

@rossberg
Copy link
Member

rossberg commented May 3, 2016

@lukewagner, I don't think that is the relevant metric. The interesting metric is: the percentage of br_if uses of arity 1 whose return value is actually used. If that is way below, say, 30% (implying that way above 70% would require a drop), then it doesn't make sense to return the value by default. Rather, have those few cases fall back to whatever more general mechanism we'll provide to consume a value multiple times.

@lukewagner
Copy link
Member

@rossberg-chromium Yes, but I think we'll have a hard time convincing ourselves that we have a reliable measurement of that metric. I feel much more confident that branch-with-value is going to be rare which, if anything, supports the original point of this issue that maybe we should, as with multi-value, hold off committing to anything (by fixing arity=0) until we have a better understanding of how code generators can actually use these features.

@kripken
Copy link
Member

kripken commented May 3, 2016

@rossberg-chromium

well, our assumption so far was that explicit dropping is only feasible if we reduce the number of "spurious" non-void operators

Sounds interesting, where is that being discussed?

@ghost ghost mentioned this pull request May 4, 2016
@sunfishcode
Copy link
Member Author

sunfishcode commented May 20, 2016

There are interesting discussions here about fixing the underlying issue here by having br_if return its value operand. However, there are open questions about how that would interacts with explicit dropping, and idea which is still being explored, and there are open questions about how producers are going to want to use it.

br_if's value operand isn't necessary for Minimum Viability, and there's significant uncertainty about how best to design it. The safe thing to do here would be to semantically limit its arity to 0, so that we don't end up spec'ing something that will conflict with future answers to the above questions.

@kripken
Copy link
Member

kripken commented May 20, 2016

If br_if values can interact with explicit dropping, then isn't it best to first have the explicit dropping discussion, then get back to this?

Otherwise making a change now seems premature, especially given dropping might be a discussion coming up soon. And removing values from br_if introduces an odd asymmetry, so we should only do it if it's properly justified, and not preemptively.

@sunfishcode
Copy link
Member Author

It's a question of priorities, I suppose. Closing this, to wait for drop operators, at which point I'll re-evaluate where we are and consider a new proposal.

@sunfishcode
Copy link
Member Author

With @kripken indicating being "ok" with this proposal, I am re-opening this PR.

As a quick summary, this issue proposes removing br_if's value operand (by fixing its arity to 0) for the MVP. We'd retain the flexibility to reintroduce the value operand in the future should we choose to.

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.

4 participants