Refactor ExpressionRunner

[dcodeIO]

Tackles the concerns raised in #2797 directly related to #2702 by reverting merging all of PrecomputeExpressionRunner into the base ExpressionRunner, instead adding a common base for both the precompute pass and the new C-API to inherit. No functional changes.

---

Current hierarchy after #2702 is

ExpressionRunner
├ [PrecomputeExpressionRunner]
├ [CExpressionRunner]
├ ConstantExpressionRunner
└ RuntimeExpressionRunner

where ExpressionRunner contains functionality not utilized by ConstantExpressionRunner and RuntimeExpressionRunner.

New hierarchy will be:

ExpressionRunner
├ ConstantExpressionRunner
│  ├ [PrecomputeExpressionRunner]
│  └ [CExpressionRunner]
├ InitializerExpressionRunner
└ RuntimeExpressionRunner

with the precompute pass's and the C-API's shared functionality now moved out of ExpressionRunner into a new ConstantExpressionRunner. Also renames the previous ConstantExpressionRunner to InitializerExpressionRunner to better represent its uses and to make its previous name usable for the new intermediate template, where it fits perfectly. Also adds a few comments answering some of the questions that came up recently.

Old hierarchy before #2702 for comparison:

ExpressionRunner
├ [PrecomputeExpressionRunner]
├ ConstantExpressionRunner
└ RuntimeExpressionRunner

cc @aheejin

[dcodeIO]

Also note that InitializerExpressionRunner in its current form seems problematic as it might support more expressions than it should (or even abort on invalid initializers instead of emitting a validation error?). This is unrelated to this PR and #2702 however and used to be this way already before, so should probably be covered in a separate PR.

[dcodeIO]

In #2702 it has been requested to initialize both maxDepth and maxLoopIterations here with = NO_LIMIT. However, the single constructor now sets these while allowing to omit either just maxLoopIterations or both. So, C++ newbie question: Are there scenarios where a derived template may never call that constructor?

[aheejin]

No I don't think there can be scenarios that these are not initialized. But I also think it is good habit to initialize member variables to some default values in general, just to prevent bugs. In this case they are initialized in the constructor so the compiled code will be the same, i.e., it's not gonna initialize them twice. I'm OK with either.

[dcodeIO]

Also, I broke CI again. But this time I'm not able to fix it apparently :(

[dcodeIO]

Also, I fixed CI again!

[aheejin]

Thank you for the quick refactoring! Sorry I still may not have the full context yet, but why are ConstantExpressionRunner and PrecomputeExpressionRunner two separate classes? It seems the functionalities you put into ExpressionRunner (and moved to ConstantExpressionRunner here) are mostly about precomputing, so what I was wondering is why we can't just improve and expose the existing PrecomputeExpressionRunner and use it in the Binaryen and C API in the first place. (This will require moving PrecomputeExpressionRunner out of Precompute.cpp, but I don't think this will be a problem)

[dcodeIO]

why are ConstantExpressionRunner and PrecomputeExpressionRunner two separate classes?

The precompute pass has a special mechanism that the C-API does not have, in that it computes a map of GetValues (known values of local.gets by reference, not by index) during the pass and uses these as additional context of the runner. The base class has similar code, but by index. An earlier attempt was to just make this an optional feature of what's now ConstantExpressionRunner, but this plus the fact that the C-API's runner must be final so we can delete its instances contributed to the general confusion during the earlier PR I believe.

It seems the functionalities you put into ExpressionRunner (and moved to ConstantExpressionRunner here) are mostly about precomputing

Yep, one way to look at this is that ConstantExpressionRunner is the new exposed PrecomputeExpressionRunner, with the original PrecomputeExpressionRunner now only adding the GetValues mechanism that is irrelevant outside of the precompute pass. But I think Constant is a slightly better name for the shared one, since it really "Executes a suspected constant expression", while the Precompute (replacement of expressions) happens in precompute only, but not in the C-API.

I was wondering is why we can't just improve and expose the existing PrecomputeExpressionRunner and use it in the Binaryen and C API in the first place

So, yeah, that's kinda what this is now after the refactor. Note that PrecomputingExpressionRunner is very tiny now, with just a constructor and the special visitLocalGet logic.

Does that make sense? :)

[aheejin]

The precompute pass has a special mechanism that the C-API does not have, in that it computes a map of GetValues (known values of local.gets by reference, not by index) during the pass and uses these as additional context of the runner. The base class has similar code, but by index.

How are the two mechanisms (GetValues-based vs. index-based) different? Do we need both?

An earlier attempt was to just make this an optional feature of what's now ConstantExpressionRunner, but this plus the fact that the C-API's runner must be final so we can delete its instances contributed to the general confusion during the earlier PR I believe.

How is a class being final relevant to whether we can delete it or not?

[tlively]

See #2702 (comment) for the context of the final thing.

Edit: We can always remove the final and add a destructor implementation. I don't think this detail should inform the overall design at all.

[dcodeIO]

How are the two mechanisms (GetValues-based vs. index-based) different? Do we need both?

Iiuc the GetValues-based mechanism is specialized to work with LocalGraph after its work is done, while the index-based approach observes in execution order. Would assume that there are ways to unify both, somehow, just not sure how large of a change that would be. Currently, precompute uses GetValues to establish an initial link of gets to values, and will then resort to the index-based approach on new sets and their respective gets during linear execution of the expression of interest. The challenge I faced in my PR was that one cannot easily compute the indexed variant from GetValues, since execution order is lost when that map is created, so one doesn't know which of all the values to initialize the runner with.

[aheejin]

Thanks for the explanation. Now I think I understand things better.

It looks CExpressionRunner is there only to be marked as final, right? Can we delete it and use ConstantExpressionRunner from C API instead? We can add an empty virtual destructor in ExpressionRunner class.

[aheejin]

No I don't think there can be scenarios that these are not initialized. But I also think it is good habit to initialize member variables to some default values in general, just to prevent bugs. In this case they are initialized in the constructor so the compiled code will be the same, i.e., it's not gonna initialize them twice. I'm OK with either.

[dcodeIO]

It looks CExpressionRunner is there only to be marked as final, right? Can we delete it and use ConstantExpressionRunner from C API instead?

Sure, let me see. How would you model this, considering that CExpressionRunner is a class, and ConstantExpressionRunner is its template? My intuition would be to make ConstantExpressionRunner a class, with a virtual visitLocalGet for PrecomputingExpressionRunner to overload, and to move ConstantExpressionRunner's implementation to src/wasm/wasm-interpreter.cpp?

[aheejin]

Hmm, yeah, come to think of it, it's not gonna be simple :( Making those methods virtual will break the static polymorphism this class hierarchy is using. I think the problem here stems from the fact that even though ConstantExpressionRunner is a parent class of PrecoputeExpressionRunner, their visitLocalGet and visitLocalSet are actually two different mechanisms. And CExpressionRunner is just a proxy-like empty class to use ConstantExpressionRunner.

How about this hierarchy, where ConstantExpressionRunner does not have local-related methods itself, and make a new class LocalExpressionRunner (I'm not sure about the name so I just threw some random name) and it will have index-based local handling methods. So ConstantExpressionRunner has common methods used between PrecomputeExpressionRunner and LocalExpressionRunner, and PrecomputeExpressionRunner has GetValues-based methods and LocalExpressionRunner has index-based methods. And C API uses LocalExpressionRunner. I'm not sure about the name so please change it if you have better ideas.

ExpressionRunner
├ ConstantExpressionRunner
│  ├ [PrecomputeExpressionRunner]
│  └ [LocalExpressionRunner]
├ InitializerExpressionRunner
└ RuntimeExpressionRunner

Do you think this makes sense? Please let me know if you have better ideas. Thanks for your time!

[dcodeIO]

Making those methods virtual will break the static polymorphism this class hierarchy is using

There's already a virtual trap() method inherited from ExpressionRunner, but I guess this is mostly about avoiding virtual methods where we can, that correct?

and PrecomputeExpressionRunner has GetValues-based methods and LocalExpressionRunner has index-based methods

Hmm, might work. So far I assumed that PrecomputeExpressionRunner still benefits from being able to fall back to the index-based variant, but might be that GetValues already covers everything and that's not necessary? If that's the case, perhaps there's indeed a way to convert initial setLocalValue values to GetValues by spawning LocalGraph prior to executing the runner (for CExpressionRunner only), also solving #2787 (maybe, didn't think this through yet).

[aheejin]

There's already a virtual trap() method inherited from ExpressionRunner, but I guess this is mostly about avoiding virtual methods where we can, that correct?

Yes. OverriddenVisitor and its children are expected to override visit*** methods and they are designed to be statically called to avoid vtable traversal.

Hmm, might work. So far I assumed that PrecomputeExpressionRunner still benefits from being able to fall back to the index-based variant, but might be that GetValues already covers everything and that's not necessary? If that's the case, perhaps there's indeed a way to convert initial setLocalValue values to GetValues by spawning LocalGraph prior to executing the runner (for CExpressionRunner only), also solving #2787.

I'm not sure what this paragraph means. I'm not suggesting to add any new functionality or LocalGraph or anything. What I meant was I was not sure why ConstantExpressionRunner has to be a parent class of PrecomputeExpressionRunner. Are there any routines that PrecomputeExpressionRunner inherit from ConstantExpressionRunner? If there is no common functionality between PrecomputeExpressionRunner and ConstantExpressionRunner, is this simpler hierarchy possible?

ExpressionRunner
├ PrecomputeExpressionRunner (`GetValues`-based local methods)
├ LocalExpressionRunner (index-based local methods + call traversal)
├ InitializerExpressionRunner
└ RuntimeExpressionRunner

[dcodeIO]

Are there any routines that PrecomputeExpressionRunner inherit from ConstantExpressionRunner

It inherits practically everything with this PR, except that it overrides visitLocalGet to inject GetValues from the precompute pass:

ExpressionRunner                    Stub with lots of "unimp"
├ ConstantExpressionRunner          Huge, common functionality of Precompute/C-API
│  ├ [PrecomputeExpressionRunner]   Tiny (ctor + visitLocalGet)
│  └ [CExpressionRunner]            Tiny (ctor)
├ InitializerExpressionRunner       Something completely different
└ RuntimeExpressionRunner           Something completely different

To me that looks quite reasonable already, and I have been under the impression that we are now trying to unify PrecomputeExpressionRunner/CExpressionRunner even more, but perhaps this is a misunderstanding? Like, PrecomputeExpressionRunner doesn't actually duplicate any code, which I believe you mentioned earlier? (Well, technically it is duplicating everything before optimizations by using a template, perhaps that's what you mean?)

[aheejin]

Sorry, nevermind my last comment. I noticed that in #2702 you moved some functionalities in PrecomputeExpressionRunner into ExpressionRunner (now ConstantExpressionRunner here).

Yeah, at this point I'm OK with this change. Thank you for spending time for this and explaining things for me!

[kripken]

Looks very nice!