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

Diff MAME version of Musashi with current #58

Open
emoon opened this issue Feb 2, 2016 · 29 comments
Open

Diff MAME version of Musashi with current #58

emoon opened this issue Feb 2, 2016 · 29 comments

Comments

@emoon
Copy link
Contributor

emoon commented Feb 2, 2016

There seems to have been a fair amount of changes over here at the MAME repro

https://github.com/mamedev/mame/commits/115ffcb10a45bca233fa6e22f674d8db8982b7df/src/emu/cpu/m68000

It might be worth taking a look and diff against the current one (here https://github.com/kstenerud/Musashi ) to make sure we don't implement something in bad way.

I don't think many of the MAME changes has made it back to the original repro.

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

Very interesting! I completely agree it's worth checking out!

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

👍

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

And the other way around, MAME still have the ADDA bug, it seems. https://github.com/mamedev/mame/blob/master/src/devices/cpu/m68000/m68k_in.cpp#L1248-L1254 and https://github.com/mamedev/mame/blob/master/src/devices/cpu/m68000/m68k_in.cpp#L1273-L1279 should evaluate OPER_AY before AX...

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Ah.. Interesting :)

This is something that one needs to make sure of to handle also:

mamedev/mame@b025f69

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

For the disassembler I found quite a bit of issues as well when I worked on this

https://github.com/aquynh/capstone/blob/next/arch/M68K/M68KDisassembler.c

I know I submitted a few changes back but I missed quite a few I'm pretty sure of

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Oh btw. Notice this clr on 000 mamedev/mame@844e696

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

I've noticed that as well, but didn't care for now, as I feel it would only affect "real-world" devices.

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Yeah I guess that is true.

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

And I guess this is taken into account with the cycle count? As I would assume this would make clr slower but I may be wrong.

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

If we where to add more accurate cycle counting based on actual RAM-accesses etc, then yes we would need to account for one less read-cycle. Today if the opcodehandler first access RAM (which should costs 4 cycles I think) and then cause an exception, we act as if the instruction was in fact "for free" and only count exception cycles. In real life, the actual cycles taken would depend on how much work the instruction had done before the exception happened, but cycles are not accounted for on that level.

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

Obviously a "fixed" implementation of CLR would do one more memory read, which would take non-zero time in the real world.

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Yeah I wonder if a move (with a dn register set to zero) would be faster in the real-world then compared to a clr

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

I might actually test it at some point and see :)

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

Ah, you meant real-world CLR, yes this "feature" makes the instruction 4 clocks slower than absolutely necessary, which you should be able to see if comparing the cycle counts for CLR in the 010, which doesn't do a useless read first.

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Yeah I just wonder if a move d0,(a0)+ would be faster than clr.w (a0)+ in the real-world.

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

Well, either way that should be evident if you compare cycle counts for the two, see the M68000UM section 8, which I don't have time to do now ;)

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Yeah, will do :)

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Looks like its taking the same amount of cycles (12)

@marhel
Copy link
Owner

marhel commented Feb 2, 2016

Oops, they have fixed the adda-bug, I now notice!

@emoon
Copy link
Contributor Author

emoon commented Feb 2, 2016

Ah :) Yeah it would be interesting to just diff the code but it seems they have C++:ified the code so that might prove a bit hard :/

@marhel
Copy link
Owner

marhel commented Feb 3, 2016

I took a look yesterday, and there seems to be history back to dec 2007, using Musashi 3.31 according to the source code. In particular, one interesting change, is fixing the dependencies on global functions preventing the use of several different Musashi-cores at the same time. Remaining global functions takes a CPU-core as an argument, I think.

Maybe, with some work, it would be possible to both use latest MAME-Musashi (4.90 i think), which I guess would include a few fixes not in https://github.com/kstenerud/Musashi, as well as remove the globals-depencencies that force us to serialize Musashi access with a MUTEX. This would improve test performance greatly, allowing multithreaded tests.

Don't know how much dependencies to the rest of MAME that has crept into the codebase, though.

@emoon
Copy link
Contributor Author

emoon commented Feb 3, 2016

Yeah that would be nice. I really never liked that Musashi has global functions with no context so yes that would be nice to get it.

As MAME is multi cpu I would imagine they keep the code fairly separate and just don't throw lots of specific stuff in there but you never know.

@emoon
Copy link
Contributor Author

emoon commented Feb 3, 2016

You didn't find any other opcode specific fixes? (I haven't looked myself)

@marhel
Copy link
Owner

marhel commented Feb 3, 2016

No,, but I didn't spend too much time looking either. It seemed that the adda fix was just a side effect of removing a lot of macros. Several fixes to 020+ though, including actually emulating 020+ and ColdFire

@emoon
Copy link
Contributor Author

emoon commented Feb 3, 2016

Alright. Good to know.

@marhel
Copy link
Owner

marhel commented Feb 3, 2016

And I think they added some more event callbacks (one for TAS in particular, which apparently uses a different bus-cycle in real-world, which can have HW-side-effects which might need to be emulated), as well as allowing bus errors to be triggered externally, and some fixes to exception stack formats IIRC.

@emoon
Copy link
Contributor Author

emoon commented Feb 3, 2016

I see. I guess some Arcades/games may have relied on it (both by intention and non-intention)

@marhel
Copy link
Owner

marhel commented Feb 3, 2016

Good guess. They specifically mentioned two games that actually depended on the TAS write not to succeed (apparantly, the callback allows the host to specify if the write should be allowed or not).

@emoon
Copy link
Contributor Author

emoon commented Feb 3, 2016

Ah I see... Must have been "fun" to track down :)

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

No branches or pull requests

2 participants