WIP memory refactorings and new_dynarec

[bobby smiles]

Hi,

I have been trying to refactor the memory module recently [1]
While it offers a saner interface, my changes also broke the new_dynarec.

That's why I'm proposing this patch [2] (to apply on top of [1]) as a starting point for fixing what I broke.
It's very WIP and experimental (so expect it to not work right away), but it might be a starting point for discussion with more knowledgeable people.

Note : I don't have an ARM device, nor a 32 bit machine to play with, so I can't even test what I propose.
That's why I'd be thankful if somebody could step up and help me here.

Regards,
Bobby

[1] https://github.com/mupen64plus/mupen64plus-core/pull/47
[2] see attached patch.

[Dorian Fevrier]

I have a Raspberry Pi (ARMv6 32bits). I also had troubles compiling the
current core because of dynarec, just tell me what you want to do.


I guess get the commit you point, apply your patch, try to compile and
give you the output? :)

This is one of the reason why I would push peoples contributes on the
main repo first.

> --
> You received this message because you are subscribed to the Google
> Groups "mupen64plus" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mupen64plus...@googlegroups.com
> <mailto:mupen64plus...@googlegroups.com>.
> To post to this group, send email to mupen...@googlegroups.com
> <mailto:mupen...@googlegroups.com>.
> Visit this group at http://groups.google.com/group/mupen64plus.
> For more options, visit https://groups.google.com/d/optout.

[bobby smiles]

Thanks for your answer !

Yeah, getting the latest commit of pull request, apply the attached patch, try to compile.
And if you're a dynarec guru hack some stuff until it compiles cleanly and run...
Or just report what you get (warnings, compile errors, ...).

I'll try to get a x86 cross compiler running and test the x86 backend on my machine.
If I can get things running with the x86 backend, hopefully someone will be able to replicate things for the ARM backend.

[Dorian FEVRIER]

Ok bsmile, I will try asap. :)

--
You received this message because you are subscribed to the Google Groups "mupen64plus" group.

To unsubscribe from this group and stop receiving emails from it, send an email to mupen64plus...@googlegroups.com.
To post to this group, send email to mupen...@googlegroups.com.

[Dorian FEVRIER]

Pff Im an idiot, I rebooted my pi without save the long mail I wrote. So here is the short version:

So, the problem seems to be on the RPi having not enough memory for compile the file...

If, with any trick, there is a way to decrease compilation memory requierement that would help (maybe using headers declaration, don't know).

If no solution is possible, I have to create a toolchain for cross plateform compilation for my pi.

But as I have no network it will not be trivial to send compiled bins to the pi and iterate testing... :(

Any advise is welcome.

$ make all
Makefile:103: Architecture "armv6l" not officially supported.'
Makefile:233: Using SDL 1.2 libraries
CC _obj/r4300/new_dynarec/new_dynarec.o
In file included from ../../src/r4300/new_dynarec/new_dynarec.c:965:0:
../../src/r4300/new_dynarec/assem_arm.c: In function ‘isclean’:
../../src/r4300/new_dynarec/assem_arm.c:408:12: warning: pointer targets in initialization differ in signedness [-Wpointer-sign]
In file included from ../../src/r4300/new_dynarec/new_dynarec.c:965:0:
../../src/r4300/new_dynarec/assem_arm.c: In function ‘alloc_arm_reg’:
../../src/r4300/new_dynarec/assem_arm.c:921:3: warning: array subscript has type ‘char’ [-Wchar-subscripts]
../../src/r4300/new_dynarec/assem_arm.c: In function ‘do_cop1stub’:
../../src/r4300/new_dynarec/assem_arm.c:3005:7: warning: unused variable ‘rs’ [-Wunused-variable]
../../src/r4300/new_dynarec/assem_arm.c: In function ‘multdiv_assemble_arm’:
../../src/r4300/new_dynarec/assem_arm.c:4267:9: warning: "/*" within comment [-Wcomment]
../../src/r4300/new_dynarec/assem_arm.c:4231:21: warning: unused variable ‘lol’ [-Wunused-variable]
../../src/r4300/new_dynarec/assem_arm.c:4230:21: warning: unused variable ‘loh’ [-Wunused-variable]
../../src/r4300/new_dynarec/assem_arm.c:4229:21: warning: unused variable ‘hil’ [-Wunused-variable]
../../src/r4300/new_dynarec/assem_arm.c:4228:21: warning: unused variable ‘hih’ [-Wunused-variable]
../../src/r4300/new_dynarec/assem_arm.c: In function ‘wb_valid’:
../../src/r4300/new_dynarec/assem_arm.c:4438:14: warning: unused variable ‘new_hr’ [-Wunused-variable]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘store_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:3021:15: warning: unused variable ‘jaddr2’ [-Wunused-variable]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘storelr_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:3211:15: warning: unused variable ‘jaddr2’ [-Wunused-variable]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘c1ls_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:3453:22: warning: unused variable ‘jaddr3’ [-Wunused-variable]
../../src/r4300/new_dynarec/new_dynarec.c: At top level:
../../src/r4300/new_dynarec/new_dynarec.c:939:17: warning: ‘ldl_merge’ defined but not used [-Wunused-function]
../../src/r4300/new_dynarec/new_dynarec.c:950:17: warning: ‘ldr_merge’ defined but not used [-Wunused-function]
../../src/r4300/new_dynarec/assem_arm.c:956:13: warning: ‘output_byte’ defined but not used [-Wunused-function]
../../src/r4300/new_dynarec/assem_arm.c:960:13: warning: ‘output_modrm’ defined but not used [-Wunused-function]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘address_generation’:
../../src/r4300/new_dynarec/new_dynarec.c:3890:9: warning: ‘ra’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘pagespan_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:6272:25: warning: ‘ntaddr’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/assem_arm.c:1025:33: warning: ‘addr’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c:6117:7: note: ‘addr’ was declared here
../../src/r4300/new_dynarec/assem_arm.c:1025:33: warning: ‘alt’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c:6117:12: note: ‘alt’ was declared here
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘storelr_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:3376:28: warning: ‘temp2’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c:3406:10: warning: ‘memtarget’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘store_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:3159:16: warning: ‘memtarget’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘load_assemble’:
../../src/r4300/new_dynarec/new_dynarec.c:2959:10: warning: ‘memtarget’ may be used uninitialized in this function [-Wuninitialized]
In file included from ../../src/r4300/new_dynarec/new_dynarec.c:965:0:
../../src/r4300/new_dynarec/assem_arm.c: In function ‘loadlr_assemble_arm’:
../../src/r4300/new_dynarec/assem_arm.c:3307:10: warning: ‘memtarget’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c: In function ‘new_recompile_block’:
../../src/r4300/new_dynarec/new_dynarec.c:9247:24: warning: array subscript is above array bounds [-Warray-bounds]
../../src/r4300/new_dynarec/new_dynarec.c:9247:43: warning: array subscript is above array bounds [-Warray-bounds]
../../src/r4300/new_dynarec/assem_arm.c:1025:33: warning: ‘ntaddr’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c:4735:20: note: ‘ntaddr’ was declared here
../../src/r4300/new_dynarec/assem_arm.c:2036:33: warning: ‘addr’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c:4735:11: note: ‘addr’ was declared here
../../src/r4300/new_dynarec/assem_arm.c:1025:33: warning: ‘alt’ may be used uninitialized in this function [-Wuninitialized]
../../src/r4300/new_dynarec/new_dynarec.c:4735:16: note: ‘alt’ was declared here
gcc: internal compiler error: Killed (program cc1)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.6/README.Bugs> for instructions.
Makefile:597: recipe for target '_obj/r4300/new_dynarec/new_dynarec.o' failed
make: *** [_obj/r4300/new_dynarec/new_dynarec.o] Error 4


--------------------------------------------
En date de : Jeu 4.12.14, 'Dorian FEVRIER' via mupen64plus <mupen...@googlegroups.com> a écrit :

Objet: Re: [mupen64plus] WIP memory refactorings and new_dynarec
À: "mupen...@googlegroups.com" <mupen...@googlegroups.com>
Date: Jeudi 4 décembre 2014, 14h26

[bobby smiles]

Thanks for your report.
Regarding your compiling issue, maybe you can try to reduce the optimization level, remove lto, or decrease warning level.
That may help to get it compile, but is not really viable in the long term.
As you said the other option is to setup a cross compiler and upload the binary to you RPi.

I've setup a x86 build environment and on my end it compiles and run but instantaneously crash with a corrupted stack, so gdb backtrace isn't very helpfull.
So don't expect this to run on your RPi for now :( 

[Dorian FEVRIER]

> Regarding your compiling issue, maybe you can try to reduce the optimization level, remove lto, or decrease warning level.

I will try this as, once again, I don't have any network to transfert bins in a smooth way. Thanks!

> I've setup a x86 build environment and on my end it compiles and run but instantaneously crash with a corrupted stack, so gdb backtrace isn't very helpfull.
> So don't expect this to run on your RPi for now :( 

When you say you've setup a x86 env. Do you mean you are not on a native x86 arch? Which arch are you?

[bobby smiles]

My main machine is an Archlinux x86_64, so it is not compatible with the new_dynarec (which is 32bit only).
I've setup a virtual machine (VirtualBox) and installed Archlinux x86 (ie 32 bit) so I can develop and test without impacting my main environment.

I've tested my patch in this environement and it results in an instantaneous crash with a corrupted stack.
So for now, unless you're going to hack the source, I guess you will have to wait for another WIP to test :(

[Dorian FEVRIER]

No problem, I will take this time to prepare a good RPi toolchain. :)

[gilles....@gmail.com]

Bobby, I fixed this here:
www.paulscode.com/forum/index.php?topic=1814.msg13563#new

[bobby smiles]

Thank you very much for the pointers on emit_movimm :)
Unfortunately, there were some other errors that prevented me to run many games.

I started over and here is the second version of the patch (x86 only).
With this version I can run most games on my 32 bit machine, unfortunately Super Mario doesn't go past the first screen...
so there is still things to fix. And I didn't implement TLB exception.

Thanks again for the help :)

[bobby smiles]

Coming with a new version of the patch :)

Now Mario64 seems to work just fine, just like other games.
The main problem was that memory handlers are now required to discard the lowest 2 bits of the address (ie word-align the address), whereas previously they were not.

Remaining things to do :
-reimplement TLB exception (do someone knows what game uses that so I can test ?)
-do the same for the ARM backend
-test to ensure no regression is present
-clean things and merge it into the pull request

[gilles....@gmail.com]

#if 0
           assert(lr>=0);
           emit_sarimm(lr,31,hr);
#else
          if(lr<0) {
            emit_loadreg(regs[t].regmap_entry[hr],hr);
          }
          else
          {
            emit_sarimm(lr,31,hr);
          }
#endif

Just out of curiosity, what does this fix?

[Dorian FEVRIER]

Good to ear improvements on your work. :)

Is there any performance improvement or it's just to make everything cleaner?

> -reimplement TLB exception (do someone knows what game uses that so I can test ?)

Goldeneye is well known to use TLB. I know there is other games relying even more on TLB than Goldeneye but I can't remember.

> -do the same for the ARM backend

On my side I still have to make my RPi dev env... :(

> -test to ensure no regression is present

I'm on a "not public yet" work to have a strong (and fast) regression test suite. It work but it's not perfect. Just point me the repo, the reference commit and the commit you want to look for regression and it should help.

I take this opportunity to ask: If you have save states reproducing a well known HLE problems (Zelda lens of thruth, Perfect Dark fisheye, etc...),just send it to me by mail and I will add it to the regression tests.

[bobby smiles]

> Is there any performance improvement or it's just to make everything cleaner?

The main reason for that refactoring was to reduce duplication and ease mantainance.
Any added performance is welcome but non intentional :)

> Goldeneye is well known to use TLB. I know there is other games relying even more on TLB than Goldeneye but I can't remember.

Thanks, I'll check it.

> I'm on a "not public yet" work to have a strong (and fast) regression test suite.

Regarding the regression suite I think it is a great idea !
If you want to test regression for the current patch, just compare the following versions:
-mupen64plus-core @7669dfe8782aa94854f9a85c50e21b95263e8fa3 (this is the commit a based my pull request on)
-mupen64plus-core @9b98afade1ea609dc738108709874339829629d0 + fix_new_dynarec_wip3.patch applied (ie last commit from pull request + latest wip patch)

Be sure to compile and run the new_dynarec with both and test the outcome.

On another note, I once tried to do "unit tests" for the RSP hle only.
It was basically a weekend project which never got upstreamed (and is not up to date anymore... but the idea can still be usefull).
If you want to take a look at it go to my mupen64plus-rsp-hle repo and look for the regtest_draft branch [1].
I even posted a message on the mailing list, but got no feedback [2].


[1] https://github.com/bsmiles32/mupen64plus-rsp-hle/commit/fc4269ec6ddb1e81ce10ab883a2db8680f835ab4
[2] https://groups.google.com/d/msg/mupen64plus/ZTphWmEFxMs/SeB-E_NbGMgJ

[bobby smiles]

>> Goldeneye is well known to use TLB. I know there is other games relying even more on TLB than Goldeneye but I can't remember.

>Thanks, I'll check it.

Just checked, and GoldeneEye seems fine (I completed the first level without noticeable issue).
However, I've found that Conker Bad Fur Day triggers TLB exceptions, so I'll work with this one.

[bobby smiles]

New WIP version, with TLB exception working !
Note that however, the TLB exception code is somewhat fragile (I play with stack pointer value...).
If someone has a better idea, please contact me.

Now the x86 backend should be on par with the version before the refactoring.

Please test and comment before I try to adapt that to the arm backend.

[Dorian Fevrier]

So, I finally been able to test you work (patch3, I need to test with
patch4). It was interesting as I realize my current regression test
design is based on master branch commit with a core/rsp/video relation.
So technically, I have done the regression test for you manually.

First, some warning during compilation:

CC _obj/ai/controller.o
CC _obj/api/callbacks.o
CC _obj/api/common.o
CC _obj/api/config.o
CC _obj/api/debugger.o
CC _obj/api/frontend.o
CC _obj/api/vidext.o
CC _obj/main/main.o
CC _obj/main/util.o
CC _obj/main/cheat.o
CC _obj/main/eventloop.o
CC _obj/main/md5.o
CC _obj/main/profile.o
CC _obj/main/rom.o
CC _obj/main/savestates.o
CC _obj/main/sdl_key_converter.o
CC _obj/main/workqueue.o
CC _obj/memory/memory.o
CC _obj/pi/controller.o
CC _obj/pi/flashram.o
CC _obj/pi/sram.o
CC _obj/plugin/plugin.o
CC _obj/plugin/dummy_video.o
CC _obj/plugin/dummy_audio.o
CC _obj/plugin/dummy_input.o
CC _obj/plugin/dummy_rsp.o
CC _obj/r4300/r4300.o
CC _obj/r4300/cached_interp.o
CC _obj/r4300/cp0.o
CC _obj/r4300/cp1.o
CC _obj/r4300/exception.o
CC _obj/r4300/instr_counters.o
CC _obj/r4300/interupt.o
CC _obj/r4300/mi.o
CC _obj/r4300/pure_interp.o
CC _obj/r4300/recomp.o
CC _obj/r4300/reset.o
CC _obj/r4300/tlb.o
CC _obj/ri/controller.o
CC _obj/rdp/core.o
CC _obj/rsp/core.o
CC _obj/si/controller.o
CC _obj/si/eeprom.o
CC _obj/si/game_controller.o
CC _obj/si/mempack.o
CC _obj/si/n64_cic_nus_6105.o
CC _obj/si/pif.o
CC _obj/vi/controller.o
CC _obj/osal/dynamiclib_unix.o
CC _obj/osal/files_unix.o
CC _obj/r4300/x86_64/assemble.o
CC _obj/r4300/x86_64/gbc.o
CC _obj/r4300/x86_64/gcop0.o
CC _obj/r4300/x86_64/gcop1.o
CC _obj/r4300/x86_64/gcop1_d.o
CC _obj/r4300/x86_64/gcop1_l.o
CC _obj/r4300/x86_64/gcop1_s.o
CC _obj/r4300/x86_64/gcop1_w.o
CC _obj/r4300/x86_64/gr4300.o
../../src/r4300/x86_64/gr4300.c: In function ‘genlb’:
../../src/r4300/x86_64/gr4300.c:1328:27: warning: unused variable
‘base2’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1328:20: warning: unused variable
‘base1’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1328:14: warning: unused variable ‘gpr2’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1328:8: warning: unused variable ‘gpr1’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c: In function ‘genlh’:
../../src/r4300/x86_64/gr4300.c:1378:27: warning: unused variable
‘base2’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1378:20: warning: unused variable
‘base1’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1378:14: warning: unused variable ‘gpr2’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1378:8: warning: unused variable ‘gpr1’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c: In function ‘genlw’:
../../src/r4300/x86_64/gr4300.c:1436:27: warning: unused variable
‘base2’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1436:20: warning: unused variable
‘base1’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1436:14: warning: unused variable ‘gpr2’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1436:8: warning: unused variable ‘gpr1’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c: In function ‘genlbu’:
../../src/r4300/x86_64/gr4300.c:1486:27: warning: unused variable
‘base2’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1486:20: warning: unused variable
‘base1’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1486:14: warning: unused variable ‘gpr2’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1486:8: warning: unused variable ‘gpr1’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c: In function ‘genlhu’:
../../src/r4300/x86_64/gr4300.c:1537:27: warning: unused variable
‘base2’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1537:20: warning: unused variable
‘base1’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1537:14: warning: unused variable ‘gpr2’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1537:8: warning: unused variable ‘gpr1’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c: In function ‘genlwu’:
../../src/r4300/x86_64/gr4300.c:1596:27: warning: unused variable
‘base2’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1596:20: warning: unused variable
‘base1’ [-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1596:14: warning: unused variable ‘gpr2’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c:1596:8: warning: unused variable ‘gpr1’
[-Wunused-variable]
int gpr1, gpr2, base1, base2 = 0;
^
../../src/r4300/x86_64/gr4300.c: At top level:
../../src/r4300/x86_64/gr4300.c:219:13: warning: ‘ld_register_alloc’

defined but not used [-Wunused-function]

static void ld_register_alloc(int *pGpr1, int *pGpr2, int *pBase1, int
*pBase2)
^
CC _obj/r4300/x86_64/gregimm.o
CC _obj/r4300/x86_64/gspecial.o
CC _obj/r4300/x86_64/gtlb.o
CC _obj/r4300/x86_64/regcache.o
CC _obj/r4300/x86_64/rjump.o
CC _obj/main/zip/ioapi.o
CC _obj/main/zip/zip.o
CC _obj/main/zip/unzip.o
CXX _obj/osd/screenshot.o
CXX _obj/osd/OGLFT.o
CXX _obj/osd/osd.o
LD libmupen64plus.so.2.0.0


And finally:

__ __ __ _ _ ____ _
| \/ |_ _ _ __ ___ _ __ / /_ | || | | _ \| |_ _ ___
| |\/| | | | | '_ \ / _ \ '_ \| '_ \| || |_| |_) | | | | / __|
| | | | |_| | |_) | __/ | | | (_) |__ _| __/| | |_| \__ \
|_| |_|\__,_| .__/ \___|_| |_|\___/ |_| |_| |_|\__,_|___/
|_| http://code.google.com/p/mupen64plus/
Mupen64Plus Console User-Interface Version 2.0.0

UI-Console: attached to core library 'Mupen64Plus Core' version 2.0.0
UI-Console: Includes support for Dynamic Recompiler.
Core: Goodname: 1080 Snowboarding (E) (M4) [!]
Core: Name: 1080 SNOWBOARDING
Core: MD5: 632C98CF281CDA776E66685B278A4FA6
Core: CRC: 58fd3f25 d92eaa8d
Core: Imagetype: .v64 (byteswapped)
Core: Rom size: 16777216 bytes (or 16 Mb or 128 Megabits)
Core: Version: 1449
Core: Manufacturer: Nintendo
Core: Country: Europe (0x50)
UI-Console Status: Cheat codes disabled.
Video Warning: No version number in 'Rice-Video' config section. Setting
defaults.
Video Warning: Old parameter config version detected : 0, updating to 1;
UI-Console: using Video plugin: 'Mupen64Plus OpenGL Video Plugin by
Rice' v2.0.0
UI-Console: using Audio plugin: <dummy>
UI-Console: using Input plugin: <dummy>
UI-Console: using RSP plugin: 'Hacktarux/Azimer High-Level Emulation RSP
Plugin' v2.0.0
Core Warning: No audio plugin attached. There will be no sound output.
Core Warning: No input plugin attached. You won't be able to control
the game.
Video: SSE processing enabled.
Video: Found ROM '1080 SNOWBOARDING', CRC 253ffd588daa2ed9-50
Video: Initializing OpenGL Device Context.
Core: Setting 32-bit video mode: 640x480
Video Warning: Failed to set GL_SWAP_CONTROL to 0. (it's 24)
Video Warning: Failed to set GL_BUFFER_SIZE to 32. (it's 24)
Video Warning: Failed to set GL_DEPTH_SIZE to 16. (it's 24)
Video: Using OpenGL: NVIDIA Corporation - GeForce GT 730/PCIe/SSE2 :
4.4.0 NVIDIA 331.113
Core: Starting R4300 emulator: Dynamic Recompiler
Core: R4300: starting 64-bit dynamic recompiler at: 0x7fa0be58ff10
Core Warning: pif_ram[0x3c] -> 00000000
Core Warning: pif_ram[0x3c] <- 00000008 & ffffffff

This fail with this error on every game I manually tested.

Next time. I will test a simple game instead of spend my time fighting
against my regression test suite script lol and realize it simply fail
at load time.

About the unit test on the RSP, it's interesting but current RSP HLE
lack of dev. :(

Reading your commit:

https://github.com/bsmiles32/mupen64plus-rsp-hle/commit/fc4269ec6ddb1e81ce10ab883a2db8680f835ab4

It seems quite intrusive, I would have isolate this in a special
unit_test folder. And as you state, such unit tests are highly dependent
of hle_t struct. Maybe a silly bit compare between two hle_t would be
enougth.

Dorian

> --
> You received this message because you are subscribed to the Google
> Groups "mupen64plus" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mupen64plus...@googlegroups.com

> <mailto:mupen64plus...@googlegroups.com>.

> To post to this group, send email to mupen...@googlegroups.com

> <mailto:mupen...@googlegroups.com>.

[Dorian Fevrier]

With patch4.

Still the warning message:

And still the problem:

Core Warning: pif_ram[0x3c] -> 00000000
Core Warning: pif_ram[0x3c] <- 00000008 & ffffffff

> --
> You received this message because you are subscribed to the Google
> Groups "mupen64plus" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mupen64plus...@googlegroups.com

> <mailto:mupen64plus...@googlegroups.com>.

> To post to this group, send email to mupen...@googlegroups.com

> <mailto:mupen...@googlegroups.com>.

[bobby smiles]

Thanks for testing and reporting bugs but I think you don't use the right codebase for testing.

>../../src/r4300/x86_64/gr4300.

>c: In function ‘genlb’:
>../../src/r4300/x86_64/gr4300.c:1328:27: warning: unused variable
>‘base2’ [-Wunused-variable]
>     int gpr1, gpr2, base1, base2 = 0;

...

>Core Warning: pif_ram[0x3c] -> 00000000
>Core Warning: pif_ram[0x3c] <- 00000008 & ffffffff

These compilation warnings and the runtime warning are all from an experimental branch I did for prototyping (refactoring_experimental branch) : it contains errors and bugs (and was not asked for PR).
If you use the code from [1], ie the one I asked for PR, you should not have these compilation+runtime warnings.
The 4th version of the fix_dynarec patch should be applied on top of [1].
=> You will get one or 2 warnings during compilation because of unused variables, but it should be okay.

>About the unit test on the RSP, it's interesting but current RSP HLE
>lack of dev. :(

There are two main reasons why it lacks dev:
-implementing HLE ucodes is hard, time-consuming and requires reverse engineering skills.
-most of common ucodes are already implemented, so not really "rewarding" given the effort required
Given these reasons, the RSP hle won't get much love for some time...

>It seems quite intrusive, I would have isolate this in a special
>unit_test folder. And as you state, such unit tests are highly dependent
>of hle_t struct. Maybe a silly bit compare between two hle_t would be
>enougth.

I wanted them to be intrusive in order to be informative : ie know where/what failed instead of just binary result "ok / failed".
Anyway, this was just a prototype made during a week-end...

While speaking of tests, another github repo might be of high interest [2] :)

[1] https://github.com/bsmiles32/mupen64plus-core/tree/refactor_memory
[2] https://github.com/PeterLemon/N64

[bobby smiles]

Sorry, I didn't saw your answer earlier...

I needed this "fix" to get super mario working...
Otherwise it would assert right after starting the rom.
I'm not really sure it is the right way to do that, but I just copy/pasted things looking on similar functions and got it "working" so I called it good enough...

Really, the new_dynarec codebase is VERY difficult for me to understand, I just do some "patch work" to get things going, but it either calls for a major refactoring or a rewrite (maybe using LLVM could help).

Anyway, I hope you'll find the latest patch usefull :)

[Dorian FEVRIER]

> These compilation warnings and the runtime warning are all from an experimental branch I did for prototyping (refactoring_experimental branch) : it contains errors and bugs (and was not asked for PR).

> If you use the code from [1], ie the one I asked for PR, you should not have these compilation+runtime warnings.

The commit hashes you gave me where not in the refactor_memory branch but in refactoring_experimental. I will test again but as it's quite time consuming (I have to do this manually for now), don't hesitate to provide me direct URL of the commits next time. :)

> I wanted them to be intrusive in order to be informative : ie know where/what failed instead of just binary result "ok / failed".
> Anyway, this was just a prototype made during a week-end...

No problem I read your code and it was interesting.

I realize something few days ago:

1) Emulator code (and HLE especially) are VERY complicate to understand as they don't follow a spec but "try to mimic" hardware.

2) Emulator code are used for years (Rice code is 2003 from the headers and I'm sure the original dev start 1 or 2 years before).

3) Because of 1 and 2, emulator code left any kind of "direction" and virtually become the property of "nobody and everybody".

4) Emulator code are bloated of "running hardware" limitation exceptions (OpenGL extension, bad OpenGL driver hacks, ARM specific etc...).

5) Because of 1,2,3 and 4 emulator codes become mess[1] with years.

6) Because 5 no one (actually only few) want to do "time consuming" contributions (and only purpose kind of salvage patchs that quickly fix the problem, never attack it from it's origin).

If there is one lesson to learn from N64 emu scene it's that an emulator code should focus on readability and portability: "Don't forget the future".

I think we should be more clean and separate the HLE code from the hardware it's running on. I've dig into the Dolphin code and, while it's far from perfect, there approach is very interesting (Basically, it's the same Principe than HACK_FOR_<game_name> applied to hardware. I think I will do the same for Rice because I don't like all this hardware hack here and there into the code.

So, technically, their will still be hardware hacks into the code but it will have to be documented directly in the code. While it seems a silly idea because "we should not worry because of the hardware and expose a parameter" to let hardware integrators dealing with it, I consider it's a shame to have a global parameter for one specific platform.

I seriously prefer a big switch with HW_SPECIFIC_QUALCOM_ANDRENO, HW_SPECIFIC_NVIDIA_TREGRA3 everywhere in the code with every enum highly documented (and not just "this is tegra") [2] instead of a global parameter future peoples would wonder why it's here and what it's supposed to be there.

Few month ago (almost one year now, wow!) I tried as much as possible to cleanup Rice code. I realize what I would expect for "past peoples". This is still a mess as there is A LOT of weird, undocumented, conditions that doesn't make senses for me but I let them here because I'm sooo affraid to break something I prefer do not touch.

So everytime I would see a salvage Rice PR that only fix a problem without clear comments or when I consider there is a better way to do this I would push to close the PR, and open an issue to discuss the problem and what is the best way to handle it.

I maybe sound a bit arsh but I think it's the good way to go with this. Notice I'm only talking about Rice code because I became familliar with it.

What do you think?

Dorian

[1] https://twitter.com/FioraAeterna/status/539224318853410817

[2] https://github.com/dolphin-emu/dolphin/blob/5c3bbf74094135b92a3f912eb67cace29676dcc3/Source/Core/VideoCommon/DriverDetails.h

 

Le Jeudi 18 décembre 2014 5h13, bobby smiles <bobby.s...@gmail.com> a écrit :

Thanks for testing and reporting bugs but I think you don't use the right codebase for testing.

>../../src/r4300/x86_64/gr4300.

>c: In function ‘genlb’:
>../../src/r4300/x86_64/gr4300. c:1328:27: warning: unused variable

--
You received this message because you are subscribed to the Google Groups "mupen64plus" group.

To unsubscribe from this group and stop receiving emails from it, send an email to mupen64plus...@googlegroups.com.
To post to this group, send email to mupen...@googlegroups.com.

[Dorian FEVRIER]

> I needed this "fix" to get super mario working...
> Otherwise it would assert right after starting the rom.
> I'm not really sure it is the right way to do that, but I just copy/pasted things looking on similar functions and got it "working" so I called it good enough...

So please comment "I need this to fix Mario 64 (not super mario right?) but it's maybe not the good way", what if you would have find such #if 0 with no info, what would you think? :)

If I understand correctly (which is very unlikely), the code was expecting for lr (load reg?) before emit_sarimm. What you did is: If it not the case emit_loadreg.

I'm not sure it's the good place and I wonder if you are not trying to fix a problem appearing elsewhere. This is why I think you should comment such "weird" code.

But I just read this few lines of code and don't have any context.

Le Jeudi 18 décembre 2014 6h06, bobby smiles <bobby.s...@gmail.com> a écrit :

Sorry, I didn't saw your answer earlier...

I needed this "fix" to get super mario working...
Otherwise it would assert right after starting the rom.
I'm not really sure it is the right way to do that, but I just copy/pasted things looking on similar functions and got it "working" so I called it good enough...

Really, the new_dynarec codebase is VERY difficult for me to understand, I just do some "patch work" to get things going, but it either calls for a major refactoring or a rewrite (maybe using LLVM could help).

Anyway, I hope you'll find the latest patch usefull :)

Le mercredi 17 décembre 2014 17:17:52 UTC+1, gilles....@gmail.com a écrit :

#if 0
           assert(lr>=0);
           emit_sarimm(lr,31,hr);
#else
          if(lr<0) {

            emit_loadreg(regs[t].regmap_ entry[hr],hr);
          }
          else
          {
            emit_sarimm(lr,31,hr);
          }
#endif

Just out of curiosity, what does this fix?

[bobby smiles]

Sorry for the wrong commit (I was probably browsing multiple branches at the same time and got confused...).
this should be better -> https://github.com/bsmiles32/mupen64plus-core/commit/296a893daac284eebbeee7b11b60b372f8002c9d

Regarding your analysis of emulator code, I kinda fell the same.
Original authors are long gone, cruft has accumulated, no one dare to touch something that currently "work", even if it is unmaintainable, hence the patch work.
I believe small refactorings are possible to an extend, but more ambitious refactoring are needed as well sometimes (and they are painfull).
That's why I proposed the "memory" refactoring which will allow to isolate each component, so we can rework them later one by one and make them unit testable.

I would as well agree on a more deep review process for commits, but the thing is that we don't have enough contributors yet to be that selective...
And we (or at least me), have not always the adequate knowledge to comment on some pull requests.

But now that there is some activity going, I am more optimistic for the project future :)

[littleguy77]

On Thursday, December 18, 2014 8:52:45 AM UTC-5, Narann wrote:

I seriously prefer a big switch with HW_SPECIFIC_QUALCOM_ANDRENO, HW_SPECIFIC_NVIDIA_TREGRA3 everywhere in the code with every enum highly documented (and not just "this is tegra") [2] instead of a global parameter future peoples would wonder why it's here and what it's supposed to be there.

Indeed that is a nice approach if you have the luxury of building from the ground up.  Hat's off to anyone with the time and energy to refactor m64p to that!

I would tweak this idea just a bit though.  I would say that platform/hardware specific hacks should be dealt with by downstream projects/front-ends whenever possible, if they exist.  For example, there are many quirks to workaround in Android and mobile hardware.  These quirks are often a constantly moving target, coming and going with each new Android release and each new chipset.  Rather than bloating upstream with a lot of (often temporary) experimental hacks, just add a config option and delegate the hacking to mupen64plus-ae since it is a large downstream project.  For the particular example of HW_SPECIFIC_QUALCOMM_ADRENO and HW_SPECIFIC_NVIDIA_TEGRA3, I don't believe those should be bloating upstream code.  Let upstream deal with broader hacks/differences, e.g. related to GL/GLES versions, Win/Linux/Mac, arm/x86, and so forth.

 

[bobby smiles]

Ok, I will add a comment there in the final pull request and in a separate commit with relevant information.

Thanks

[gilles....@gmail.com]

I needed this "fix" to get super mario working...
Otherwise it would assert right after starting the rom.

 
I merged your latest patch but without this fix and I can't reproduce what you supposedly fixed here. If this is not happening with master I think the problem might be somewhere else.

Note that however, the TLB exception code is somewhat fragile (I play with stack pointer value...).
If someone has a better idea, please contact me.

The only solution I can think of is to remove all "emit_writeword_imm_esp" and store the "instr addr + flags" inside a variable instead of using the stack.

Something like that:

emit_writeword_imm(start+i*4+(((regs[i].was32>>rs1[i])&1)<<1)+ds,(int)&instr_addr_flag);

and change

tlb_exception:
    /* eax = cause */
    /* ebx = address */
    /* ebp = instr addr + flags */
    mov    0x24(%esp), %ebp

by:

tlb_exception:
    /* eax = cause */
    /* ebx = address */
    /* ebp = instr addr + flags */
    mov    instr_addr_flag, %ebp

   

Wrapper which allow to JUMP (not call !) tlb_exception from linkage_*.S

I think this isn't necessary take a look at new_dyna_start for an example of an assembly function called from C.

[Dorian Fevrier]

> Hat's off to anyone with the time and energy to refactor m64p to that!

More I read Dolphin code, more I think it's the way to do. Keeping all
those exposed config parameters for very few specific hardwares seems
bloating to me compared to the purpose and let the code with some
branching future contributors will have no way to understand. I consider
if a value have to be exposed, the code will have to explain clearly in
which situation this parameter could be removed.

I will create a ticket to discuss this. We _could_ have a shared
definition between all video plugins but share hardware definitions for
video plugins in the mupen64plus API seems kindof facepalming...

Yet another lesson to learn from N64 emu scene: Do never, ever, write
plugin based emulator as, the day no one care about the device you
emulate (aka, N64 today), you fall with a huge amount of similar codes
to maintain with nobody to do the job...

> just add a config option and delegate the hacking to mupen64plus-ae
since it is a large downstream project.

The problem I have with exposing parameters for downstream each time a
driver is not working properly is that it separate the video plugin code
knowledge from the hint of why this parameter have to be exposed. When a
future contributor will read the code an see an exposed parameter
controlling a critic code part, he will have no idea why it's exposed
(some games need specific values? is it driver related?), bringing to a
code hard to maintain. For the time I dive into Rice there is a bunch of
HUGE branching depending on exposed parameters I have no idea why they
are here and I don't even know if it's relevant anymore... With a
DRIVER_BROKEN_<hardware_name> you have no question about the hint, you
know why the branching is here because the DRIVER_BROKEN_<hardware_name>
enum is commented.

My purpose is to integrate this "knowledge" inside the upstream code. If
some chipsets are broken, I prefer to have a
DRIVER_BROKEN_<hardware_name> inside the code making the reader aware
about why the code is branching here. The same we did for HACK_FOR_GAME.
This way, upstream devs can choose if they support a specific hardware
or not.

Basically, the question is: Who should deal with bad driver crap?
upstream or downstream?

Few options:
1) The upstream code have to be clean and portable. Aka: If your
hardware is bad, contact your vendor. (pros: code maintainable and
clean, con: a lot of bad hardware will not run mupen64plus smoothly).
2) The upstream shouldn't care about the crappy driver but provide a
config way on demand to help downstream peoples to deal with by them
self. (it's basically the current way but I don't like it for points
explained above)
3) The upstream should deal with bad driver in a way that would make new
bad driver fix easy to add for downstream. (Dolphin)

As you guess, 3 is my favorite option. I consider downstream projects
should have base code to integrate fixes nicely and potentially upstream
this as needed making the hack benefice to every downstream project.

What do you think?

Dorian

> --
> You received this message because you are subscribed to the Google
> Groups "mupen64plus" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mupen64plus...@googlegroups.com

> <mailto:mupen64plus...@googlegroups.com>.

> To post to this group, send email to mupen...@googlegroups.com

> <mailto:mupen...@googlegroups.com>.

[bobby smiles]

> I merged your latest patch but without this fix and I can't reproduce what you supposedly fixed here. If this is not happening with master I think the problem might be somewhere else.

! Big shame on me !
I 've found that the libmupen64plus I was using to compare against was not compiled with NEW_DYNAREC=1 so basically I was comparing my WIP work against the "old dynarec".
=> What that means is that, in fact this "fix" is unrelated to the memory refactoring work !
So it will go in another pull request, so we can discuss it separately, and see why you can't reproduce that on your machine.

> The only solution I can think of is to remove all "emit_writeword_imm_esp" and store the "instr addr + flags" inside a variable instead of using the stack.

That would be a start, but this is not sufficient because the tlb_exception isn't a "regular" function.
It plays with the stack (add $0x24, %esp) and does not "return" but "jump" to get_addr_ht(0x80000180).
Before the refactoring, memory handlers (read_nomem_new, ...) were written in ASM and were guaranteed not to use the stack (only global variables and registers),
so that was not a problem. But now that I changed the memory handlers prototypes (1 return value, 2 or 3 parameters) and rewrote them in C with possibly variables on the stack,
the stack pointer has to be adjusted before jumping to tlb_exception and before jumping to get_addr_ht(0x80000180).

== ("less") Ugly hack suggestion ==
Maybe, instead of jumping to get_addr_ht(0x80000180) from the tlb_exception,
we can "replace" the return address of the {read,write}{b,h,w,d} functions with get_addr_ht(0x80000180) when exceptions occurs.
Ie deliberately replace the return address written on the stack.


I will try that later to see if it works.
== End of suggestion ==

Again thanks for your invaluable help on that :)

[bobby smiles]

I think I got something a little bit less fragile :
instead of adjusting manually the stack pointer like I did before, I store the esp value before the call and use that value to restore a suitable esp value before jumping to tlb_exception.

See patch v5.


If this solution is considered good enough, I will start to work on the ARM backend and see if I can reuse the C functions I introduced.

[bobby smiles]

Slight "optimization" : I moved the call to goto_tlb_exception inside {read,write}_nomem_new instead of {read,write}{b,h,w,d}.

[littleguy77]

On Friday, December 19, 2014 12:01:52 AM UTC-5, Narann wrote:

I will create a ticket to discuss this. 

Please do.  The bandwidth is too low on this mailing list... sometimes it takes 24 hours for my posts to get through and most readers are probably not interested.

[Dorian FEVRIER]

I didn't forgot you bsmile32! I will try to regression test your paches asap! :)

Instead of using patches, I would recommend to use your own branch. Once your work is done, use rebase to merge some commits:

https://stackoverflow.com/questions/19178869/how-to-merge-two-git-commits-into-one

That's just suggestions but this would help me as I would just have to clone your branch and build without patching stuff. :D

Keep the good work!

Le Vendredi 19 décembre 2014 7h31, bobby smiles <bobby.s...@gmail.com> a écrit :

Slight "optimization" : I moved the call to goto_tlb_exception inside {read,write}_nomem_new instead of {read,write}{b,h,w,d}.

--
You received this message because you are subscribed to the Google Groups "mupen64plus" group.

To unsubscribe from this group and stop receiving emails from it, send an email to mupen64plus...@googlegroups.com.
To post to this group, send email to mupen...@googlegroups.com.

[bobby smiles]

I have not used my branch during the WIP stage because I expected this to be very iterative process, and I wanted to avoid "rewriting history" often.
Now that I'm mostly happy with the end result for the x86 backend, I've pushed that into the original pull request [1]..

I will try work on the ARM backend this week-end, but this will be challenging because I don't have an ARM device to test with (nor a an ARM cross-toolchain to begin with).
Is there any detailled tutorial available to setup such environnement ?

Otherwise, it wil be just guess work from source code and delegated testing :(

[1] https://github.com/mupen64plus/mupen64plus-core/pull/47

[gilles....@gmail.com]

I got some time to work on the memory refactor this afternoon and I think the best way to this is to port everything to assembly so we can get rid of such ugly patch:

 

static inline void goto_tlb_exception(uint32_t cause, uint32_t address)
{
#if NEW_DYNAREC == NEW_DYNAREC_X86
        asm volatile (
                "mov g_mem_saved_stack_pointer, %%esp\n"
                "sub $4, %%esp\n"
                "jmp tlb_exception\n"
                : /* no output */
                : "a"(cause), "b"(address)
                : /* no clobber */);
#else
#error TODO: goto tlb_exception not implemented
#endif
}

emit_writeword(ESP, (int)&g_mem_saved_stack_pointer);

Here's my work so far:
http://pastebin.com/PZq31KSE

This is mainly a proof of concept, it's not entirely tested and it's definitely not well optimized.
Also as I'm working on VS this is intel format assembly so this need to be translated to AT&T in order to be merged in the current linkage_x86.s, sorry about that :-(

[Dorian FEVRIER]

Putting everything to ASM is maybe not a bad choice for such purpose but I would push for adding more comments because such code is never maintain because it's very hard to read. Please add as much comments as possible to explain the "hint" of what you are doing because if a bug appear and someone have to read in you asm he will never known what you where thinking about.

What I humbly suggest is to add, in comment, a roughly C equivalent of what you are doing. This make the code logic easy to read while leaving the complexity of ASM from the mind of the reader. Example:

mov     ecx,    [_g_mem_address] ;addr1 = _g_mem_address
mov     ebx,    ecx              ;addr2 = addr1
shr     ebx,    16               ;addr2 = addr2<<16
mov     eax,    [ebx*4+_readmem] ;proc1 = addr2*4+_readmem (why *4?)

etc...


Specifically, function calls can be hard to get in asm. I like how they comment function calls:

http://www.cs.virginia.edu/~evans/cs216/guides/x86.html

Search for "The code below shows a function call" I think every function call in asm should be commented.

Another good comment to keep a map of what/why you are shifting stuff: https://en.wikibooks.org/wiki/X86_Assembly/Shift_and_Rotate

I know it can be boring but it's asm code, please consider the future contributors. You have everything in mind and from a certain perspective, your knowledge of this code is valuable! :)

Keep the good work! :)

Regards,

Dorian