Merge HandlerLowering into a single file#533
Conversation
|
Nice simplification! @CAG2Mark do you think you coudl leave a review (approving or requesting changes) one of these days? |
I can try |
|
|
||
| lazy val scopedVars: collection.Set[ScopedSymbol] = this match | ||
| case Scoped(syms, body) => syms ++ body.scopedVars | ||
| case _ => this.subBlocks.foldLeft(Set.empty)((vars, blk) => vars ++ blk.scopedVars) |
There was a problem hiding this comment.
And use this.subBlocks.iterator to avoid the unnecessary creation of intermediate sets.
There was a problem hiding this comment.
I thought flatMap require convert the list to a set first (this.subBlocks is a list). I guess I can use iterator + flatMap + toSet
There was a problem hiding this comment.
If you use .toSet instead of .iterator/.toSet, you create an intermediate set. With toSet, a (probably) more efficient builder will be used to build the result set in one go. Not entirely sure this really makes a real difference one way or the other, though.
There was a problem hiding this comment.
I wonder if we should have tail-call optimization be run after lifting, but before the effect handler transformation.
There was a problem hiding this comment.
In fact I want to move effect handler transformation to be right before JSBuilder to enjoy all the optimization. Currently effect handler benchmark have a big disadvantage compared to non-instrumented code as the instrumentation completely obfuscate the control flow, and elimination of certain variable that is live at the effectful result become impossible since they will be saved for unwinding.
There was a problem hiding this comment.
Good point. We just have to make sure none of the optimization passes assume the absence of funny semantics due to effect handler. But it may be that this is already the case. Not sure.
There was a problem hiding this comment.
I guess we can do this in a follow-up PR – IIRC we already talked with @AnsonYeung about how to refactor the compilation pipeline into a cleaner CompilationPipeline class, moving things out of Lowering.
There was a problem hiding this comment.
My intuition is that they should just work. For example, abortive effects are essentially Throw except they behave even nicer due to the absence of finally and that the body is always a separate function. This means that as long as exceptions are handled properly they should be handled as well. The transformation of resumption will put the resumed value into the correct variable as well
There was a problem hiding this comment.
Actually, we should probably add support for finally for effect handlers at some point 🤓
This is actually important if we implement exceptions as effects and handlers, for compatibility reasons.
Co-authored-by: Mark Ng <55091936+CAG2Mark@users.noreply.github.com>
3 participants
Splitting into several files caused difficulty when the code of HandlerLowering changes, stack safety have a lot of assumption about the inner workings of HandlerLowering already and we can avoid a retraversal on the tree since stack safety and handler lowering uses the same analysis information. The HandlerLowering already have several special cases because of stack safety. This also make both pass more consistent as evident by improvement of at least one test case where StackSafetyTransform did not check EffectfulResult before, and now it share the information with HandlerLowering directly.
This also fixes a bug: if the stack safety effect is raised right when the function is resumed by runtime, unwind will wipe out the pc and local variable state due to stack safety always unwind the function to the initial state. Since the pass is now merged, moving the check to after the restoreVars section is easy.
As an added bonus, I removed the forced while loop rewriting for handlers. Since lifting is always done when handler is turned on, loop variables can be safely floated out to the top. This will not cause any semantic change as no nested lambda exist and thus no code can access inner loop variables from the previous iteration.