-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Lift address-bus width limit to 64 bits. #14689
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
base: master
Are you sure you want to change the base?
Conversation
|
@MooglyGuy I will take a proper look in a couple of weeks, unfortunately can't until then. PS: I fat-fingered the "ready for review" button, sorry. |
|
I have had a quick look, and none of the changes stand out in any of my stuff as being problematic. FWIW, Alpha also needs >32-bit physical addresses, and this will allow unwinding an address-space hack. From memory, even mips3 has 34(?) bits of physical space which we currently don't handle. |
| break; | ||
| default: | ||
| fprintf(stderr, "%s: read from bad offset %d\n", __FILE__, offset); | ||
| fprintf(stderr, "%s: read from bad offset %04x\n", __FILE__, (uint16_t)offset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I’d change things like this to:
util::stream_format(std::cerr, "%s: read from bad offset %04x\n", __FILE__, offset);That way you get type safety, etc. from the strformat.h magic and don’t need the casts.
(This one in particular should be a logerror though, and not have the file name in it – it shouldn’t be sending that stuff to stderr.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be happy to address the string_format and stream_format things as a bulk change some other time, as there's a lot more than just these handfuls of spots that need reworking. There are commented-out printfs that have obviously long-since rotted, there are printfs that definitely could be logmacro, a whole bunch of that sort of thing that would be better addressed separately. The intent behind my changes was simply to address each compile error in turn as it came up.
| TMS340X0_TO_SHIFTREG_CB_MEMBER(isa16_ex1280_device::to_shiftreg) | ||
| { | ||
| printf("address to shiftreg: %08x\n", address); | ||
| printf("address to shiftreg: %08x\n", (uint32_t)address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for this – to send stuff to standard output with type safety, you can use:
util::stream_format(std::cout, "address to shiftreg: %08x\n", address);But the stuff in this device should probably be logmacro.h stuff, not sending stuff to standard output anyway.
| data = bitswap<8>(data ^ 0x22, 2, 6, 7, 4, 3, 1, 5, 0); | ||
|
|
||
| unsigned idx {bitswap<4>(offset, 8, 5, 2, 1)}; | ||
| uint64_t idx {bitswap<4>(offset, 8, 5, 2, 1)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The narrowing is safe in this case since the result is only ever four bits – not sure how much performance impact making idx 64 bits would have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safe or not, it's a compile error on Windows GCC as it weas written. So, you tell me: Should I make it a uint64_t, or should I cast to an unsigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it onlyv getting upset because it uses uniform initialiser syntax? Does it also get upset by:
unsigned idx = bitswap<4>(offset, 8, 5, 2, 1);etc.?
Compilers are weird about when they give narrowing warnings and when they don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this new file supposed to be part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly, it was not.
|
Keep in mind this will need some non-trivial DRC surgery:
|
As-is it requires no DRC surgery at all, so any of those changes can be tackled without much time pressure. My aim here is to be able to take a stab at an interpreter core for MIPS R10000, which has a 40-bit physical address bus. Unless someone is sitting on a DRC for a CPU that has an address bus width over 32 bits, it should not be perceived as a blocker. |
It will still break i686 because the stack layout for calling address space read/write methods changes when the address changes from 32 bits to 64 bits. On Windows i686, the stack pointer will end up at the wrong place because of Thinking about it, this also affects the calls to the debugger instruction hook that the back-end generates, as that has an This will break all CPUs using a recompiler whether they want bigger address spaces or not. |
So I simply hallucinated sf2049, panicprk, xfiles, and windheat all booting and running just fine. Got it. |
|
Sounds to me like it's a good reason to get rid of the i686 recompiler. The only reason we kept it was that it was no annoying enough to maintain yet. Blocking 64-bits addresses is annoying enough in my book. |
This switches the
offs_tdeclaration fromu32tou64, updates the global mask inaddress_map::map_validity_check, and fixes the compile errors that fell out of the change.Most of it was associated with logging, so I added explicitly-narrowing casts where most applicable, and switched other situations over from using
u32for addresses to usingoffs_t.It would be nice if @pmackinlay would have a look, as ROMP, NS32xxx, and M88x00 had more changes than most.