binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

`BrOn` does not support carrying additional values

Open tlively opened this issue 2 years ago • 3 comments

The spec allows all branches to carry an arbitrary number of values to their destination labels (as long as the destination label has a result type that expects those values). We represent this with an Expression* values field in the Break class, but the BrOn class has no such field and does not support carrying any values except the inspected reference. We should fix this by adding an Expression* values field to BrOn as well.

tlively avatar May 15 '23 19:05 tlively

For spec correctness we need this, in principle, but I'd say it's very low priority to add. In fact I'd recommend leaving it for now, as adding it will make the program use multivalue blocks, since these instructions already forward one value - any more and we have >1. Multivalue blocks are are still not optimized well, so the risk is that if we add this field to BrOn before we optimize it well then people could see regressions. For now I think we are ok, as no one has requested this field be added, and people using wasm-opt will get a clear error on it not being supported, and code we do not error on is all well-optimized.

kripken avatar May 15 '23 20:05 kripken

Yes, I agree this is low priority. In principle it would be nice to match the spec, though. There's a bit of a chicken-and-egg problem if we don't implement features because people aren't using them, because people won't use them if we don't implement them.

tlively avatar May 15 '23 21:05 tlively