Repository navigation
ExpressionRunner class hierarchy #2797
Description
#2702 exposes functionalities of ExpressionRunner to Binaryen and C API. I find most usages there is about precomputing certain expression, and we have PrecomputeExpressionRunner for that. But after #2702 some functionalities of PrecomputeExpressionRunner are duplicated in ExpressionRunner to make them exposed to Binaryen and C API, and I'm not very sure what the difference between ExpressionRunner and PrecomputeExpressionRunner is at this point.
So I'm not sure the functionalities of ExpressionRunner class hierarchy is very well defined now. Can't Binaryen and C API make use of PrecomputeExpressionRunner instead, if the main purpose of that is precomputing?
I might be mistaken, so please advise me if so :)
Activity
dcodeIO commentedon Apr 23, 2020
Yeah, that's a refactor I'm not especially proud of. Essentially, what happened there is that most of
PrecomputeExpressionRunnerhas been refactored into the baseExpressionRunner, since an earlier attempt to movePrecomputeExpressionRunnertowasm-interpreter.hasStandaloneExpressionRunner(extendingExpressionRunner) turned out to be overly confusing in review. So the suggestion of moving most of it into the base class was made and implemented. There's also the situation now where the baseExpressionRunnerhas some fields that are never used by the derivedRuntimeExpressionRunner, which I mentioned on the original PR as it seems unfortunate.The underlying concern for extending
PrecomputeExpressionRunnerfromExpressionRunnerthen was that it does something special withgetValues(what seems to be duplicate code), which are being computed during the precompute pass andCExpressionRunnerdoesn't need.CExpressionRunnerexists because it must befinalfor some C reasons.Perhaps we should pick up the idea again to add a class in between, what previously was
StandaloneExpressionRunner(ContextualExpressoinRunner?ContextAwareExpressionRunner?LinearExpressionRunner?StaticExpressionRunner?), that onlyPrecomputeExpressionRunnerandCExpressionRunnerinherit from, but notRuntimeExpressionRunner?aheejin commentedon Apr 23, 2020
I didn't review that PR, so could you elaborate on what parts were overly confusing in doing that?
dcodeIO commentedon Apr 23, 2020
Sure, the pivotal discussion was #2702 (comment), then leading to that change.
aheejin commentedon Apr 24, 2020
Sorry, I read the comment and am still at a loss.
PrecomputeExpressionRunnerinstead so Binaryen and C API can use it?dcodeIO commentedon Apr 24, 2020
This has implications on the way we track intermediate values of locals (which is one of my additions that precompute didn't have previously). If execution was not be linear, we cannot simply use a map that we write to on
local.set, and read onlocal.get. Somewhat analogous to the runner exclusively operating in execution order.See also #2804 (comment) on the other thread. Mostly boils down to the precompute pass having one special mechanism.
kripken commentedon Apr 24, 2020
I'd support a refactoring here, I agree this should be cleaner. Hopefully we can find a nice solution!
Sorry I didn't push on this more in reviewing that PR. Part of the reason is I had "tunnel vision" in a large and long-running review of a PR - over time in review I focused less on the big picture and more on specific details. But in a big refactoring like that I should have re-read the entire thing more frequently.
dcodeIO commentedon Apr 24, 2020
Can we please agree that this is all my fault for being terrible at C++ and too inexperienced with Binaryen's codebase, making my PR's hard to review in the first place? :) I promise this will improve a lot over time, when I become more confident! (I guess it also takes a little longer than usual since I'm external and mostly relying on somewhat indirect, so sometimes inefficient means of communication.)
Refactor ExpressionRunner (#2804)
aheejin commentedon Apr 28, 2020
Addressed by #2804. Thanks!