From an old email thread: The baseline compiler currently maintains its own control stack in parallel with the ExprIter because it occasionally needs access to elements below the top one: all branchy control flow (br, brIf, brTable) needs to find the target label for the block and also information about the execution stack depth at that target. The ion compiler has something similar in its blockPatches_ vector. It's not any kind of hardship to maintain this information, but if the controlItem() accessor in the ExprIter were to take a relativeDepth argument (0 == top) then I could almost certainly reuse the ExprIter's control stack. Of course, it's possible the ExprIter doesn't want to expose that it in fact has a stack structure here, though it seems less avoidable for control items than for value items that there be a stack. To which Dan and Luke both replied: > This seems fine to me. Dan additionally said: > The control-flow stack structure is unlikely to change significantly unless wasm itself changes (eg. to support irreducible control flow), and if wasm changes, than the baseline compiler's own control-flow stack would need to change anyway, regardless of how much we encapsulate things in ExprIter. Indeed I was looking at this code recently and it still seems possible to use the iterator's control stack. I'm not sure if it's *worthwhile* to do this simplification, but we should investigate. Sharing code is good.

Having bumped up against this a few times now, I'm definitely in favor of this simplification and making any generalizations necessary to ExprIter to allow baseline to reuse its control-flow-stack logic.

(I think this might be another good polish bug to do before tiering/shipping.)

Make it so.

This is mostly straightforward. There are two complications. (1) The iterator pops its control stack in readEnd, and this discards an item that the compiler needs after that, so we must make a copy. (I assume readEnd can't be deferred until after we've processed the node, in case Verification is interleaved with compilation.) The copy is no big deal, our ControlItem is small. (2) The iterator discards the control item when it switches from the 'then' to the 'else', and this does not work for the baseline compiler. I have chosen here to add an option to the iterator's readElse() not to do that, but I could instead have chosen to make a copy before the call to readElse() and then copy back after that call. It is also possible that discarding the element is not actually necessary, but it's hard to know what the contract is here. WasmIonCompile is not obviously dependent on the reinitialization at this time.

Actually we need to dig deeper into point (2) above. In the patch I just attached to bug 1329096, the ControlItem is not copyable (because it contains a substructure with a deleted assignment operator, a NonAssertingLabel). That means that the assignment of an empty control item in switchToElse() can't work. So we need to make the call on whether that reinitialization needs to be there, and if it needs to be there whether it needs to be an assignment or whether it can be something else. (Clearly for the present patch I need the reinitialization not to happen, since I want the value to persist.)

I seem to understand what we want is a ControlStackItem with the same type and valueStackAtStart, and we're resetting in the switchToElse method to avoid to have a pop/push sequence on the controlStack_: so this looks like the right place to do this, Dan is probably a better person to have an opinion here. Using a pointer would be impractical here, so making controlItem_ a Maybe<ControlItem> could do: if (reinitControl) { controlItem_.reset(); controlItem_.emplace(ControlItem()); }

I think we can just remove the reinitialization of the controlItem_ altogether. WasmIonCompile.cpp is currently the only code making use of the controlItem_, and it doesn't need the controlItem_ reinitialized in readElse. In fact, it could even be slightly simplified if we remove the reinitialization, because it's currently making a copy of the data before its readElse call. Concerning (1), if ControlItem is going to become noncopyable, then copying the controlItem_ before the readEnd call won't work. One option would be to split readEnd into two calls, one which does the validation and one which does the controlStack_.popBack().

Here's a quick sketch of your patch plus switchToElse's reinitialization removed plus readEnd split into readEnd and popEnd.

(In reply to Dan Gohman [:sunfish] from comment #7) > I think we can just remove the reinitialization of the controlItem_ > altogether. WasmIonCompile.cpp is currently the only code making use of the > controlItem_, and it doesn't need the controlItem_ reinitialized in > readElse. In fact, it could even be slightly simplified if we remove the > reinitialization, because it's currently making a copy of the data before > its readElse call. Oh, excellent. > Concerning (1), if ControlItem is going to become noncopyable, then copying > the controlItem_ before the readEnd call won't work. One option would be to > split readEnd into two calls, one which does the validation and one which > does the controlStack_.popBack(). Yes, I sort of worked around that with a Move() in the patch on the other bug but I worry about the soundness of that, really. There are many reasons for something not to be copyable, but having its address taken is one of them, and Move() would break invariants in that case. Thanks for the patch, I will apply + test + resubmit for review.

Comment on attachment 8827823 [details] [diff] [review] bug1316814-use-iterators-ctl-stack.patch Let's just do feedback? for now.

Comment on attachment 8827823 [details] [diff] [review] bug1316814-use-iterators-ctl-stack.patch Review of attachment 8827823 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks great! I like Dan's suggestion above regarding splitting readEnd(). Also, considering the simplifications (removing freeStack()) in bug 1329096, it might even simplify reviewing this patch to fold that patch into this one.

(In reply to Luke Wagner [:luke] from comment #11) > Comment on attachment 8827823 [details] [diff] [review] > bug1316814-use-iterators-ctl-stack.patch > > Review of attachment 8827823 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall, looks great! I like Dan's suggestion above regarding splitting > readEnd(). It shall be done. > Also, considering the simplifications (removing freeStack()) in > bug 1329096, it might even simplify reviewing this patch to fold that patch > into this one. IMO that's a complication - the issues are orthogonal and freeStack() is just a small part of it - so I'll keep the bugs and their patches separated. New patches coming up all around.

Ground work: This is Dan's patch reduced to only splitting readEnd and popEnd in the iterator, and with users of the iterator updated.

Very similar to the version you just saw, but builds on the split readEnd / popEnd to avoid copying the ControlItem, and does not make any changes to the iterator code, only to the baseline compiler.

Comment on attachment 8827977 [details] [diff] [review] bug1316814-split-readEnd-from-popEnd.patch Review of attachment 8827977 [details] [diff] [review]: ----------------------------------------------------------------- Code looks good. Trying to read through the readEnd->popControl->mergeControl calltree (understanding what each of these distinct levels means so as to understand the consequences of this intermediate post-readEnd/pre-popEnd state we're now in), it seemed to be that it'd be more clear to: - inline popControl() into its one caller, readEnd(); it also makes a bit more sense when you have the context of knowing you're just talking about `end` - rename mergeControl() to checkEndOfBlock() (or some other name) to indicate that we're not mutating the control state (which is what had me worried); we're just doing end-of-block validation If you agree, could you include these superficial changes?

Comment on attachment 8827980 [details] [diff] [review] bug1316814-use-iterators-ctl-stack-v2.patch Review of attachment 8827980 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, looks good, thanks! ::: js/src/wasm/WasmBaselineCompile.cpp @@ +410,5 @@ > bool deadOnArrival; // deadCode_ was set on entry to the region > bool deadThenBranch; // deadCode_ was set on exit from "then" > }; > > + struct BaseCompilePolicy : OpIterPolicy { nit: { on new line @@ +3910,5 @@ > > + void endBlock(ExprType type, Control& block); > + void endLoop(ExprType type, Control& block); > + void endIfThen(Control& ifThen); > + void endIfThenElse(ExprType type, Control& ifThenElse); With the readEnd/popEnd split, can you remove these second arguments and use currentItem() inside these respective functions?

Luke's suggestions in comment 15 sound good to me. Also, it doesn't sound like it's important here, but ControlItems currently do have to be Moveable because we currently store them in a Vector which resizes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1571ba44e85026e0e5ded9ebf4df5f31371cff68 Bug 1316814 - Split wasm iterator's readEnd and popEnd. p=sunfish, r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/9042d15381359dc12bbce404f157cdde53e359eb Bug 1316814 - wasm baseline, use the iterator's control stack. r=luke

https://hg.mozilla.org/mozilla-central/rev/1571ba44e850 https://hg.mozilla.org/mozilla-central/rev/9042d1538135