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

Here lies JitIL, a long-time CPU engine of Dolphin. #1023

Closed
wants to merge 1 commit into
base: master
from

Conversation

@waddlesplash
Contributor

waddlesplash commented Sep 7, 2014

Rationale for removing it:

  • It was only faster than Jit64 on 32-bit x86 which is no longer supported and has been removed
  • After the flag optimization landed, JitIL did not get it and as such is broken on >80% of games (according to @JMC47).
@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Sep 7, 2014

Member

You will need to also remove the references to JitIL from the VS filter file

Member

lioncash commented Sep 7, 2014

You will need to also remove the references to JitIL from the VS filter file

Here lies JitIL, a long-time CPU engine of Dolphin.
Rationale for removing it:
  * It was only faster than Jit64 on 32-bit x86 which is no longer
    supported and has been removed
  * After the flag optimization landed, JitIL did not get it and as
    such is broken on >80% of games (according to JMC4789).
@waddlesplash

This comment has been minimized.

Show comment
Hide comment
@waddlesplash

waddlesplash Sep 7, 2014

Contributor

@lioncash done.

Contributor

waddlesplash commented Sep 7, 2014

@lioncash done.

@Sonicadvance1

This comment has been minimized.

Show comment
Hide comment
@Sonicadvance1

Sonicadvance1 Sep 7, 2014

Contributor

Need to update the Android UI to remove references to it as well.

Contributor

Sonicadvance1 commented Sep 7, 2014

Need to update the Android UI to remove references to it as well.

@archshift

This comment has been minimized.

Show comment
Hide comment
@archshift

archshift Sep 7, 2014

Contributor

The rationale for keeping it was that it could help find bugs on JIT.

Contributor

archshift commented Sep 7, 2014

The rationale for keeping it was that it could help find bugs on JIT.

@waddlesplash

This comment has been minimized.

Show comment
Hide comment
@waddlesplash

waddlesplash Sep 7, 2014

Contributor

@Sonicadvance1 will do.

@archshift indeed, but now since nobody seems keen on maintaining it and it doesn't boot the vast majority of games I don't see any reason to keep it around.

Contributor

waddlesplash commented Sep 7, 2014

@Sonicadvance1 will do.

@archshift indeed, but now since nobody seems keen on maintaining it and it doesn't boot the vast majority of games I don't see any reason to keep it around.

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Sep 7, 2014

Member

Ping @JMC47. As the main tester for things, is JITIL still useful for finding bugs in the JIT or no?

Member

lioncash commented Sep 7, 2014

Ping @JMC47. As the main tester for things, is JITIL still useful for finding bugs in the JIT or no?

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 7, 2014

Member

I like the JitIL approach. It's a good start which would allow you to do optimisations which are hard to impossible in jit64.

But it needs lots of work and maintenance to reach it's full potential, which it just isn't getting.

The only compelling argument (apart from testing) I can see for keeping it in the tree is that if someone did want to start maintaining it, re-merging it would be hard. (And that's not the best argument.)

Member

phire commented Sep 7, 2014

I like the JitIL approach. It's a good start which would allow you to do optimisations which are hard to impossible in jit64.

But it needs lots of work and maintenance to reach it's full potential, which it just isn't getting.

The only compelling argument (apart from testing) I can see for keeping it in the tree is that if someone did want to start maintaining it, re-merging it would be hard. (And that's not the best argument.)

@delroth

This comment has been minimized.

Show comment
Hide comment
@delroth

delroth Sep 7, 2014

Member

Kind of sad to see it go tbh, but recently it's been lagging behind and nobody seems really interested in maintaining it.

The ideas behind it are great and imo I think it could potentially have been better than our current JIt in many ways, but it always lacked manpower to fix regressions.

So +0.5 from me.

Member

delroth commented Sep 7, 2014

Kind of sad to see it go tbh, but recently it's been lagging behind and nobody seems really interested in maintaining it.

The ideas behind it are great and imo I think it could potentially have been better than our current JIt in many ways, but it always lacked manpower to fix regressions.

So +0.5 from me.

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 7, 2014

Member

I am half interested in maintaining and improving JitIL myself, but I don't know if I will ever get around to doing so.

Member

phire commented Sep 7, 2014

I am half interested in maintaining and improving JitIL myself, but I don't know if I will ever get around to doing so.

@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Sep 8, 2014

Contributor

Unless someone is planning on making it boot games again, I don't see why it shouldn't be removed. Should we wait a while? Should we see if anyone is interested? Flagsopt still has a few games broken on JIT, too.

FWIW, I was misunderstood. Flagsopt was implemented on JITIL, it just doesn't seem to be working well.

Contributor

JMC47 commented Sep 8, 2014

Unless someone is planning on making it boot games again, I don't see why it shouldn't be removed. Should we wait a while? Should we see if anyone is interested? Flagsopt still has a few games broken on JIT, too.

FWIW, I was misunderstood. Flagsopt was implemented on JITIL, it just doesn't seem to be working well.

@skidau

This comment has been minimized.

Show comment
Hide comment
@skidau

skidau Sep 8, 2014

Contributor

@magumagu think you could have a look at the issues in JitIL?

Contributor

skidau commented Sep 8, 2014

@magumagu think you could have a look at the issues in JitIL?

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 8, 2014

Member

Lets not rush to remove this. It's not currently hurting the rest of our code base.

I would like some time to think about if it's worth keeping/fixing/improving. Then perhaps we should talk about this in our next monthly progress report, see if someone appears to maintain it.

How about a deadline of halfway through October before removal?

Member

phire commented Sep 8, 2014

Lets not rush to remove this. It's not currently hurting the rest of our code base.

I would like some time to think about if it's worth keeping/fixing/improving. Then perhaps we should talk about this in our next monthly progress report, see if someone appears to maintain it.

How about a deadline of halfway through October before removal?

@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Sep 8, 2014

Contributor

Maybe remove the option from the UI for users; since right now it's an absolutely huge liability.

Contributor

JMC47 commented Sep 8, 2014

Maybe remove the option from the UI for users; since right now it's an absolutely huge liability.

@lioncash

This comment has been minimized.

Show comment
Hide comment
@lioncash

lioncash Sep 8, 2014

Member

I think that would remove incentive to even consider bringing it back to a more operational state.

Member

lioncash commented Sep 8, 2014

I think that would remove incentive to even consider bringing it back to a more operational state.

@magcius

This comment has been minimized.

Show comment
Hide comment
@magcius

magcius Sep 8, 2014

Member

I don't want someone to maintain it just enough so we can keep it in and then promptly drop it again. I want a commitment to maintenance, and if nobody has been doing that now, I don't see why it would start.

It's not like a revert is particularly hard to do if we want the code back -- it's all still there in the git history.

Member

magcius commented Sep 8, 2014

I don't want someone to maintain it just enough so we can keep it in and then promptly drop it again. I want a commitment to maintenance, and if nobody has been doing that now, I don't see why it would start.

It's not like a revert is particularly hard to do if we want the code back -- it's all still there in the git history.

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 8, 2014

Member

The problem with reverting is that it becomes hard to git blame. A bunch of
history becomes hard to access.

It also (like removing the UI option) kills most of the incentive to get it
working.


Scott Mansell

On 8 September 2014 17:30, Jasper St. Pierre notifications@github.com
wrote:

I don't want someone to maintain it just enough so we can keep it in and
then promptly drop it again. I want a commitment to maintenance, and if
nobody has been doing that now, I don't see why it would start.

It's not like a revert is particularly hard to do if we want the code back
-- it's all still there in the git history.


Reply to this email directly or view it on GitHub
#1023 (comment).

Member

phire commented Sep 8, 2014

The problem with reverting is that it becomes hard to git blame. A bunch of
history becomes hard to access.

It also (like removing the UI option) kills most of the incentive to get it
working.


Scott Mansell

On 8 September 2014 17:30, Jasper St. Pierre notifications@github.com
wrote:

I don't want someone to maintain it just enough so we can keep it in and
then promptly drop it again. I want a commitment to maintenance, and if
nobody has been doing that now, I don't see why it would start.

It's not like a revert is particularly hard to do if we want the code back
-- it's all still there in the git history.


Reply to this email directly or view it on GitHub
#1023 (comment).

@comex

This comment has been minimized.

Show comment
Hide comment
@comex

comex Sep 8, 2014

Contributor

I wish less were predicated on the limitations of git blame - not only this but formatting changes. I don't even have an easy way, when blaming, to move to the version of the file before the last (frequently unrelated) change to a given line, although apparently tig can do that...

Contributor

comex commented Sep 8, 2014

I wish less were predicated on the limitations of git blame - not only this but formatting changes. I don't even have an easy way, when blaming, to move to the version of the file before the last (frequently unrelated) change to a given line, although apparently tig can do that...

@magcius

This comment has been minimized.

Show comment
Hide comment
@magcius

magcius Sep 8, 2014

Member

We commonly make giant changes and refactors to the source code like the removal of Src/, and I can still use git blame just fine. We shouldn't delete garbage crummy code if it's garbage crummy code just so someone can spend 5 less seconds figuring out who was responsible for the garbage crummy code they want to delete.

The incentive to fix JITIL has been there for at least half a year, and nobody has shown up. Why would they be incentivized now, and who will do it?

Member

magcius commented Sep 8, 2014

We commonly make giant changes and refactors to the source code like the removal of Src/, and I can still use git blame just fine. We shouldn't delete garbage crummy code if it's garbage crummy code just so someone can spend 5 less seconds figuring out who was responsible for the garbage crummy code they want to delete.

The incentive to fix JITIL has been there for at least half a year, and nobody has shown up. Why would they be incentivized now, and who will do it?

@magcius

This comment has been minimized.

Show comment
Hide comment
@magcius

magcius Sep 8, 2014

Member

FYI, you can copy/paste the revision of who brought it back, and use git blame my_revision_here^ -- Src/JITIL/Garbage.cpp. It's not difficult.

Member

magcius commented Sep 8, 2014

FYI, you can copy/paste the revision of who brought it back, and use git blame my_revision_here^ -- Src/JITIL/Garbage.cpp. It's not difficult.

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 8, 2014

Member

So, Turns out JitIL is 100% broken in current master.

I like JitIL. So I'm going to spend some time fixing it.

Member

phire commented Sep 8, 2014

So, Turns out JitIL is 100% broken in current master.

I like JitIL. So I'm going to spend some time fixing it.

@waddlesplash

This comment has been minimized.

Show comment
Hide comment
@waddlesplash

waddlesplash Sep 8, 2014

Contributor

Well, @phire seems to have fixed it, for now anyway, and it apparently has merit that I didn't know about. Closing this (for now at least).

Contributor

waddlesplash commented Sep 8, 2014

Well, @phire seems to have fixed it, for now anyway, and it apparently has merit that I didn't know about. Closing this (for now at least).

@magumagu

This comment has been minimized.

Show comment
Hide comment
@magumagu

magumagu Sep 9, 2014

Contributor

I'm not sure JITIL worth maintaining at this point... Jit64 has become much more mature since JITIL was written, JITIL's floating-point handling code needs substantial changes now that we have a better understanding of how the Gekko FPU works, the most important issues in terms of CPU optimizations in the near future aren't the sort of things JITIL actually helps with, and it seems like I'm the only person who really understands how JITIL works.

I've also learned a lot about software development since I initially wrote JITIL: the fact that JITIL is a fork makes it much more difficult for it to get anywhere. If someone is actually interested in reviving JITIL, the right approach is probably to kill the existing code and redo it incrementally on top of JIT64. A patch introducing an IR almost identical to the PPC instruction set (same operations, abstract registers) plus a simple register allocator would be large, but probably still small enough to merge without forking the JIT, and it would mostly use the existing well-tested codepaths. With that basic framework in place, we could start making the IR for some important operations like integer arithmetic and branches more abstract, and adding some cross-instruction optimizations.

Contributor

magumagu commented Sep 9, 2014

I'm not sure JITIL worth maintaining at this point... Jit64 has become much more mature since JITIL was written, JITIL's floating-point handling code needs substantial changes now that we have a better understanding of how the Gekko FPU works, the most important issues in terms of CPU optimizations in the near future aren't the sort of things JITIL actually helps with, and it seems like I'm the only person who really understands how JITIL works.

I've also learned a lot about software development since I initially wrote JITIL: the fact that JITIL is a fork makes it much more difficult for it to get anywhere. If someone is actually interested in reviving JITIL, the right approach is probably to kill the existing code and redo it incrementally on top of JIT64. A patch introducing an IR almost identical to the PPC instruction set (same operations, abstract registers) plus a simple register allocator would be large, but probably still small enough to merge without forking the JIT, and it would mostly use the existing well-tested codepaths. With that basic framework in place, we could start making the IR for some important operations like integer arithmetic and branches more abstract, and adding some cross-instruction optimizations.

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 9, 2014

Member

I'm not sure JITIL is worth maintaining at this point either.

My interest in maintaining JITIL right now mostly involves the possibility of either progressively transforming it's IR to something closer to what we need or Straight ripping the IR out and plopping a new one in. I feel like such an approach could be easier than starting from scratch again, or doing it incrementally (But I only started looking at the code seriously last night.)

If someone is actually interested in reviving JITIL, the right approach is probably to kill the existing code and redo it incrementally on top of JIT64. A patch introducing an IR almost identical to the PPC instruction set (same operations, abstract registers)

This is actually happening right now. Fiora's Carry optimisation patches (see #1021) are pushing PPCAnalyst to the point where it is treating the powerpc instruction stream (and the InstructionInfo data) as a rudimentary IR. Carry optimization is exactly the kind of optimisation that should have been implemented in JITIL. Theoretically, the code would probably look cleaner than what is getting implemented in Jit64, with it's convoluted instruction re-ordering and instruction merging.

Fiora's Carry optimisation PR and other PRs like it are actually the most compelling reason for ditching JITIL.

I may be putting some maintenance into JitIL, but the removal argument is not over.

Member

phire commented Sep 9, 2014

I'm not sure JITIL is worth maintaining at this point either.

My interest in maintaining JITIL right now mostly involves the possibility of either progressively transforming it's IR to something closer to what we need or Straight ripping the IR out and plopping a new one in. I feel like such an approach could be easier than starting from scratch again, or doing it incrementally (But I only started looking at the code seriously last night.)

If someone is actually interested in reviving JITIL, the right approach is probably to kill the existing code and redo it incrementally on top of JIT64. A patch introducing an IR almost identical to the PPC instruction set (same operations, abstract registers)

This is actually happening right now. Fiora's Carry optimisation patches (see #1021) are pushing PPCAnalyst to the point where it is treating the powerpc instruction stream (and the InstructionInfo data) as a rudimentary IR. Carry optimization is exactly the kind of optimisation that should have been implemented in JITIL. Theoretically, the code would probably look cleaner than what is getting implemented in Jit64, with it's convoluted instruction re-ordering and instruction merging.

Fiora's Carry optimisation PR and other PRs like it are actually the most compelling reason for ditching JITIL.

I may be putting some maintenance into JitIL, but the removal argument is not over.

@waddlesplash

This comment has been minimized.

Show comment
Hide comment
@waddlesplash

waddlesplash Sep 9, 2014

Contributor

Well, in that case...

Contributor

waddlesplash commented Sep 9, 2014

Well, in that case...

@waddlesplash waddlesplash reopened this Sep 9, 2014

@phire

This comment has been minimized.

Show comment
Hide comment
@phire

phire Sep 9, 2014

Member

Yes. Leave it open. This could be a long drawn out argument.

The fact that JitIL is not going to survive in it's current state is certain. The question is does it get transformed, or deleted (and when exactly should it be deleted?)

Member

phire commented Sep 9, 2014

Yes. Leave it open. This could be a long drawn out argument.

The fact that JitIL is not going to survive in it's current state is certain. The question is does it get transformed, or deleted (and when exactly should it be deleted?)

@magcius

This comment has been minimized.

Show comment
Hide comment
@magcius

magcius Sep 9, 2014

Member

I think everybody agrees that the long-term goal is to merge the strategy and ideas of JITIL with JIT64. Having watched Fiora's changes, yeah, JIT64 is growing to the point where it needs an IR.

I don't think we need to keep around the JITIL code because of git blame simply so that we can merge it in. I've also had users / developers get confused -- "wait, JITIL is the fancy new experiemental JIT, right? Why are you working on JIT64? Shouldn't I be working on JIT64?" Killing JITIL is a step towards progress, I feel.

Users who need JITIL because games don't work under JIT64 now have to get angry and file bugs and we have to fix JIT64 and make it better and stronger.

Member

magcius commented Sep 9, 2014

I think everybody agrees that the long-term goal is to merge the strategy and ideas of JITIL with JIT64. Having watched Fiora's changes, yeah, JIT64 is growing to the point where it needs an IR.

I don't think we need to keep around the JITIL code because of git blame simply so that we can merge it in. I've also had users / developers get confused -- "wait, JITIL is the fancy new experiemental JIT, right? Why are you working on JIT64? Shouldn't I be working on JIT64?" Killing JITIL is a step towards progress, I feel.

Users who need JITIL because games don't work under JIT64 now have to get angry and file bugs and we have to fix JIT64 and make it better and stronger.

@delroth delroth closed this Sep 13, 2014

@waddlesplash waddlesplash deleted the waddlesplash:kill-jitil branch Oct 24, 2014

@waddlesplash waddlesplash referenced this pull request May 20, 2017

Merged

Remove JITIL #5445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment