ykjit / yk

yk packages
https://ykjit.github.io/yk/
Other
28 stars 7 forks source link

Enable deoptimisation of inlined constants. #1247

Closed ptersilie closed 1 month ago

ptersilie commented 1 month ago

I'm raising this as a DRAFT, as I'm not quite sold on the design yet and would like to have some additional eyes on this. There's two main issues: 1) We need to introduce a LocalAlloc::Const, which doesn't make much sense conceptually. 2) We currently don't adjust for the size of the constant (this can be easily fixed by adding the size to the enum variant)


Due to function inlining (and later other optimisations) it can happen that a variable in AOT becomes a constant in the JIT. Currently, neither the trace builder nor deoptimisation can handle such cases, which this commit fixes, by inserting a ProxyConst instruction and reference this instead of the direct constant.

ltratt commented 1 month ago

I have a couple of comments that are probably worth addressing, but overall I'd say: let's just do this. It might turn out to be imperfect, and then we can iterate on it in-tree.

ltratt commented 1 month ago

The other thing we'll want to add to this PR is at least one unit test to x86_64/mod.rs for the const case.

ptersilie commented 1 month ago

The other thing we'll want to add to this PR is at least one unit test to x86_64/mod.rs for the const case.

Sure. Can the JIT IR parser deal with ProxyConsts or do I need to add that first?

ltratt commented 1 month ago

Can the JIT IR parser deal with ProxyConsts or do I need to add that first?

Good point. No, it doesn't deal with them because I originally thought "they can't appear in the output therefore they shouldn't be parsed". But I think that was wrong: they should be parsed even though they can't appear in the output. I would suggest %0: i8 = 1i8esque syntax for this.

[I think we're tending in a direction where ProxyConst will become AssignConst, but we can worry about that later.]

ptersilie commented 1 month ago

Yeah I'm also starting to think a normal assign instruction isn't a bad idea. It would make the special LocalAlloc::Const case go away too.

ltratt commented 1 month ago

For this PR I think I would stick with ProxyConst just because there will be more knock-on effects from changing it than are first apparent, and such a change is probably best done in isolation.

ltratt commented 1 month ago

I think it's just Const -> ConstInt and the unit test (and JIT IR parser) change needed to get this in.

ptersilie commented 1 month ago

This should address the remaining issues.

ltratt commented 1 month ago

Please squash and mark the PR as "ready for review".

ptersilie commented 1 month ago

Squashed.

ptersilie commented 1 month ago

Force pushed some C formatting changes.

ptersilie commented 1 month ago

Sorry, forgot to run the other tests. Doing so revealed that the instruction printing needed changing to deal with proxy stuff.

ltratt commented 1 month ago

Hmm, not totally convinced by that new patch, but I'm also unsure what the right thing should be! Let's get it in and work out what to do later.

Please squash.

ptersilie commented 1 month ago

Yeah, I must admit I wasn't too sure either.

Squashed.