Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for deopts in promises #1177

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Support for deopts in promises #1177

wants to merge 8 commits into from

Conversation

JanJecmen
Copy link
Collaborator

If there is a deoptimization in a promise (that is also the outermost frame, ie.,
forcing a promise that isn't inlined), we cannot longjump out of the optimized
execution, since the promise doesn't have its own context.

Thus we add a return instruction immediately after the deopt, which is only reached
if there was no longjump. In that case, we know that this is the evaluation of
a promise that deoptimized, and the result is left on the stack to be returned.

TODO: actually use speculation in promises (most passes don't run on promises as of now)

@o-
Copy link
Contributor

o- commented Feb 16, 2022

Watch out for this annotation here which needs to be removed, or actually only put there if it is a non-promise: https://github.com/reactorlabs/rir/blob/master/rir/src/compiler/native/builtins.cpp#L2448

@JanJecmen
Copy link
Collaborator Author

Oh, I just removed it.. maybe have two versions of deoptImpl then, one for promises and one for closures

@o-
Copy link
Contributor

o- commented Feb 16, 2022

Oh, I just removed it.. maybe have two versions of deoptImpl then, one for promises and one for closures

sorry I somehow completely missed that you did already.

but yeah, it should help llvm with optimizations. you can also just add attributes in code, no need to have two versions. (of course not, its on the function, not the call...)

maybe drop the attribute, but keep the unreachable for the cases where it is not a promise? that should be equally good as the no return annotation.

@JanJecmen JanJecmen force-pushed the prom-deopt branch 2 times, most recently from 79a10dd to f750863 Compare May 19, 2022 12:43
@o-
Copy link
Contributor

o- commented May 19, 2022

Here are some stats for your PR:

  • WARNING: the longest CI job test_features_3 took 2.59h
  • fastaredux_naive regressed by 0.9
  • lapplyDots regressed by 0.91
  • matrixFor regressed by 0.94
  • Overall benchmarks regressed by 1.0

Please find your performance results at https://rir-benchmarks.prl.fit.cvut.cz/diff?job_ids[]=2479130138&job_ids[]=2467128976&selection=all

Because of an optimization where we replace the first checkpoint in an inlinee by the checkpoint at the callsite it is possible to take the outer deopt exit from within the inlinee. In case the inlinee needs its own context we need to pop it, otherwise the context offsets in MkEnv instructions in the deopt block would modify the wrong frame.
```
a = checkpoint -> [deopt with (mkenv ... context 1)]
...
c = push_context ....
assume ..., a -> we have to pop c
```
The other option would be to make the context field in MkEnvs a PIR variable and then set it to the correct value based on where we come from, however, this seems easier.
This adds support for deopting from promises that are the outermost frame when deopting.
The mechanism is that if the outermost frame is a promise, we don't longjump to a context
(since promises don't have contexts), but rather return the result. This is the only case
when deopt returns, and we immediately return also from the promise code with the result,
passing it to the place where the promise forcing occured.
@o-
Copy link
Contributor

o- commented Jun 17, 2022

Here are some stats for your PR:

  • WARNING: the longest CI job test_big_inline took 3.29h
  • Bounce_nonames_simple regressed by 0.93
  • nbody_naive regressed by 0.74
  • emptyFor regressed by 0.86
  • Overall benchmarks improved by 1.0

Please find your performance results at https://rir-benchmarks.prl.fit.cvut.cz/diff?job_ids[]=2599407256&job_ids[]=2578982488&selection=all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants