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
DSPHLE: Support EDuke32 Wii libaesnd uCode #10892
DSPHLE: Support EDuke32 Wii libaesnd uCode #10892
Conversation
|
Tested to work in eduke32. |
| inline bool SwapLeftRight() const; | ||
| inline bool UseNewFlagMasks() const; | ||
|
|
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.
inline is useless here, you're telling the compiler that these methods may be defined in multiple translation units but not actually providing the implementation in the header so it never will be.
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.
Does inline in AESnd.cpp have meaning, and if so, does using inline in AESnd.cpp require also using it AESnd.h? I know I've done it in the cpp file before and I did it in the header as well, but I'm not 100% confident I understand the rules surrounding it.
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'm not actually sure what a conforming C++ compiler/linker does when you use different linkage declarations across multiple declarations/definitions on the same function (MSVC appears to use the one in the declaration rather than the definition), but to clear up a likely misunderstanding here: inline does not mean that the function will be inlined (or considered for inlining), it just affects linkage. (See the first two answers here for details.) The way you've used it here doesn't do anything in practice because there's no possible second compilation unit that could contain a definition.
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.
Thanks. In particular this was useful:
Generally, the compiler will be able to do this better than you. However, the compiler doesn't have the option to inline code if it doesn't have the function definition. In maximally optimized code usually all private methods are inlined whether you ask for it or not.
I made a similar mistake in #10826; I'll change it locally there too. (I have a few other ideas for changes I want to make to that now that I've read the ppc 750cl manual further, to better support dcache emulation later on. So I've made it a draft for now.)
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.
(We do also have DOLPHIN_FORCE_INLINE in Common/Inline.h, but I think that's overkill here.)
565e2e9
to
6c9b24c
Compare
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.
Code looks sensible. I assume the fifoci changes are unrelated.
6c9b24c
to
9315706
Compare
9315706
to
5de6734
Compare
| @@ -293,6 +293,7 @@ std::unique_ptr<UCodeInterface> UCodeFactory(u32 crc, DSPHLE* dsphle, bool wii) | |||
|
|
|||
| case 0x008366af: | |||
| case 0x078066ab: | |||
| case 0x5ad4d933: | |||
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.
Just curious, is there a reason these aren't using constants?
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 don't think there's any reason not to other than that there aren't super obvious names for them (I've got constants in ASnd.cpp and AESnd.cpp, but AX.cpp/AXWii.cpp and Zelda.cpp don't have them and there isn't any easy way to get more information about them either).
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.
Ok, I didn't know if it'd be helpful to put the constant here instead. But this function is mostly non-constants with comments. So maybe just a comment instead?
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 decided to try out the constants, and it ended up being fairly easy to move these ones to the header file, so I've switched to using them. That should make it easier to find the more detailed comments associated with the constants. (I don't think it'd be useful to change the other ones, apart from maybe card and GBA - note that UCODE_ROM and UCODE_INIT_AUDIO_SYSTEM are magic values that aren't actual hashes, which is kinda jank.)
This version is exclusive to EDuke32 Wii (see https://bugs.dolphin-emu.org/issues/12990).
5de6734
to
1dcccb1
Compare
The first version is exclusive to EDuke32 Wii (see https://bugs.dolphin-emu.org/issues/12990). The second version fixes a bug with
MAIL_TERMINATE(that hasn't been merged into libogc, and currently only exists in Extrems' libogc2). Like with #10793, I did some checking to confirm that the source and binaries matched, and also checked padding behavior (see aesnd_ucode_versions_extra.zip).Hashes of various possible versions for search purposes