#189 ROMCS handling broken with more than one romcs peripherals

Gergely Szasz

If more than one peripherale share the ROMCS line than they clash with the "last" because there is only one romcs memory_map (memory_map_romcs[]).

Here is two patch:
- first change the common memory_map_romcs[] to local memory_map_romcs[]s for every peripherials
- the second change memory_map_read[] and memory_map_write[] to pointers


  • Gergely Szasz
    Gergely Szasz

  • Gergely Szasz
    Gergely Szasz

  • I think that the "romcs" memory pages should be dynamically allocated as required from the memory pool (e.g. see if2_from_snapshot() ) rather than being statically allocated in each periph file.

  • Gergely Szasz
    Gergely Szasz

    Hmm... the data itself dynamically allocated of course (the patch does not change this area at all), only the administration area (the memory_page struct) 'multiplicated' in order to each periph have their own.

  • I see, you are correct and I was confused.

    The only additional point I have is that it would be nice if each "periph" ROM block was named for the peripheral (e.g. in if2.c memory_map_romcs -> if2_memory_map_rom or similar) - I'll have a pass at doing so.

  • Committed a slightly amended fuse.romcs_a00.diff in revision 4099.

    I have looked at fuse.romcs_b00.diff and it does exactly as you say, changes memory_map_read and memory_map_write to pointers, but I wasn't sure what the benefit was? I don't see any particular downside either, but I wondered what motivated the change?

  • "what the benefit was?"
    only, we do not copy structs (needlessly), just pointers. (4 byte instead of 40)

  • In this case I think we are likely swapping an uncommon 40 byte copy when changing paging layout for a pointer indirection every time we check for contention on memory reads and writes - have you done any profiling to check whether this is a performance win?

  • "have you done any profiling..." didn't ... :(
    but we already use indirection when we check contention on memory read/write:

    readbyte( libspectrum_word address )
    libspectrum_word bank;
    memory_page *mapping;

    bank = address >> 13;
    mapping = &memory_map_read[ bank ];

    if( debugger_mode != DEBUGGER_MODE_INACTIVE )
    debugger_check( DEBUGGER_BREAKPOINT_TYPE_READ, address );

    if( mapping->contended ) tstates += ula_contention[ tstates ];
    tstates += 3;

    if( opus_active && address >= 0x2800 && address < 0x3800 )
    return opus_read( address );

    return mapping->page[ address & 0x1fff ];

    ... the writebyte() is quite identical too...

  • All the following routines are macros as they are performance-critical, and we would be adding extra indirection here. I'd be happy to apply if we can demonstrate that we get a performance benefit:

    /* Get the appropriate contended memory delay. Use a macro for performance
    reasons in the main core, but a function for flexibility when building
    the core tester */

    #ifndef CORETEST

    #define contend_read(address,time) \ if( memory_map_read[ (address) >> 13 ].contended ) \ tstates += ula_contention[ tstates ]; \ tstates += (time);

    #define contend_read_no_mreq(address,time) \ if( memory_map_read[ (address) >> 13 ].contended ) \ tstates += ula_contention_no_mreq[ tstates ]; \ tstates += (time);

    #define contend_write_no_mreq(address,time) \ if( memory_map_write[ (address) >> 13 ].contended ) \ tstates += ula_contention_no_mreq[ tstates ]; \ tstates += (time);