nthnn / rishka Goto Github PK

You could probably make these specializations of an overload and really help the optimizer just pull these inline without the overhead of a function call. You're seemingly using type punning here to make your deal with the optimizer clear and keep alignment, etc. OK (a lovely approach!) but making it an overloaded operator, you really could make this a zero-cost abstraction.

For some reason, code of this type (including my own) always tends to look more like C with a salting of C++ that sometimes forgets to take advantage of all the things that C++ can offer. Don't forget about things like constexpr and overloads that can really help the readability to both you and teh optimizer.

These might even reduce to a lowly cast if you have control of the memory layout of both the input and outut here...

If rishka_virtual_machine were a class had a constructor or init style class that initialized it to a guaranteed sane state, you might be able to save yourself some maintenance/synchronization grief.

The zeroing of registers[], for example, could probably reduce to a memset, probably inlined. Then if you were targeting, say, RV32E that had half as many RV registers, you wouldn't need to special-case that '32', the ctor would just set the appropriate registers.

This is very nice code, but don't let your structures be afraid to blossome out into classes with all the convenience and performance that goes with that. In fact, elsewhere in the code, it shows that you're not afraid of C++..., so don't be afraid to let your C++-flag fly freely!

I'd default running to false, zero-initialize registers[] (and provide accessors for things like debuggers or viewers...), ensure that stream*'s life cycle is always appropriate, etc.

You don't have to include something from every chapter of https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines in order just to prove you're worthy (you've produced a meaningful base of code - you're clearly worthy!) but don't be afraid to let the C++ bits poke through. and take advantage of th safety and performance that's available to you. Let rishka_syscall (and the rest of instructions*) be class enums for better type safety and so your own debugging code can offer more introspective views, etc.

It's clean and nifty code, but it looks like C code without the type safety and promises allowed by "Modern C++".

(And if you're tired of my opinionated bullshit, feel free to justcan all of these suggestions... :-) )

I've not called out every occurrence of this in the code and I know that in panic()-style code it's tempting to handle byte writes as robotically as possible so you get every dying breath of debug info out, but generally you want to reduce the overhead of round-tripping to any system below you.

This code, for example, might benefit from

https://www.arduino.cc/reference/en/language/functions/communication/serial/write/

So

void rishka_perror(const char* msg, uintptr_t len, bool flush) {
    if(len > 0 && msg)
        for(int i = 0; i < len; i++)
            Serial.write(msg[i]);

    if(flush) {
        Serial.println();
        Serial.flush();
    }
}

might become

void rishka_perror(const char* msg, uintptr_t len, bool flush) {
if (flush)
  Serial.println(str);
  Serial.flush();
} else {
  Serial.write(msg.len);
}

If this is in a templatable environment and flush is a constant (it almost always is) the optimizer may be able to remove the runtime test of flush totally and just replace your perror() with a calle to Serial.write(msg, len) so the correct message is passed through, though you might find the flexibility of making it more of a printf() call that evaluates arguments instead of a raw write() call that can potentially just load up a few pointers to the DMA engine and let your DMAC move it out.

But this specific call is a bit of overkill/distraction. The reaching point is that you want to batch those calls and deal in single calls with large (larger than a single byte) arguments instead of individual call/return sequences. Think "puts()" instead of "putc()". If the String returned by readString() already has a fast member that knows the length, don't get the c_str() and then traverse THAT looking for that terminator. (In some buffering system, getting the c_str itself can be costly if the buffers have to be coalesced.) If write() is just doing a store (what about flow control?) this kind of thing is probably not totally hideous, but if you're packetizing that into a debug protocol by building up a remote debug packet or programming a DMA transfer for a UART or ethernet packet, it can easily be a thousand or more times the overhead.

Optimizing a crash error message on its own is dumb, but there were enough places in the code that processed data a byte at a time to make it seem worth typing this. When emulating a CPU, misspent clock cycles can sneak up on you quickly!

Save the pounds ounces at a time. If you're not from Myanmar, Liberia, or other Metric-impaired world, just translate that cliché from using "a lot" and "a little". :-) Try to make function calls with batched values (std::spans, vector, etc.) instead of individual member/byte-level data if you at all can.

Cool project - carry on!