..:: PCSX2 Forums ::..

Full Version: Linux patches review
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Pages: 1 2 3 4 5
Just a thread to do some reviewing of my patches (or others) before breaking everythings Wink
Here the first patch.

Some inline cause some failure in Gcc. When there is no symbol problem, I replace __forceinline by __inline. So gcc just inline the function wihtout complaining... In the others case I remove __forceinline because it cause some missing symbol failure (if gcc inline it by __inline)...

Note: I have the problem with both cmake and codeblock (Release mode). Am I the only one impacted ?
For the sake of compatibility, I guess we could settle with _inline in all cases.
It's just that MSVC seemed to take _inline as a mildly funny suggestion, and ignore it.
Maybe better to wait and see what Jake has to say.. Tongue2
In fact I think inline depend on the standard but also on compiler extension which is the main reason what is a mess.

Gcc behavior is strange, may be a check with gcc-4.5 could be a good idea. Well gcc is some situation must also take it as a suggestion. The reason is inlining tend to increase binary size and thus increase cache pressure. Just do a quick test I remove Attribute always inline, the code size drop from 4.8MB to 4.1MB (inline = +17%).

In case we replace all __forceinline with _inline expect link failure everywhere in linux build. I thinks it is related to extern inline which does not work as expected.

Here an example of clamp_mix function (which is not inline in linux currently)
* Mixer.cpp. The function is defined here and used.
* Mixer.h contains the header with extern keyword
* Reverb.cpp use also the clamp_mix functions

The issue is after the compilation of mixer.cpp into object. The symbol clamp_mix does not exists (because the function is inline the function does not exist). So there is a missing symbol in Reverb.cpp.
(06-05-2010, 05:08 PM)gregory Wrote: [ -> ]Well gcc is some situation must also take it as a suggestion. The reason is inlining tend to increase binary size and thus increase cache pressure. Just do a quick test I remove Attribute always inline, the code size drop from 4.8MB to 4.1MB (inline = +17%).

Yeah, and that's pretty well irrelevant on today's CPUs (anything with 1MB L2 or better). We did extensive testing on inlining, and we can't find a single scenario where inlining aggressively penalizes PCSX2 performance. Even in what seemed to me entirely brutal code cache pressure, the __forceinline performance won over. MSVC tends to be really conservative about inlining in general, which is why we started pasting it all over creation. And sure enough, the more __forceinlines we used, the faster the emu performed.

Quote:Here an example of clamp_mix function (which is not inline in linux currently)
* Mixer.cpp. The function is defined here and used.
* Mixer.h contains the header with extern keyword
* Reverb.cpp use also the clamp_mix functions

Unless I'm mistaken, we have situations like that all through PCSX2; which uses __forceinline on hundreds of functions. Why does it only cause problems in SPU2-X?
OH, I know why, I bet. GCC gets buggy when you mix __fastcall and __forceinline. I used to do that back when I first started working on SPU2-X, because I still didn't trust MSVC to inline aggressively even with __forceinline. Turns out it almost always inlines, so the __fastcalls can be safely removed. The calling convention should never matter anyway with proper inlining.
I agree with CPU cache. And l3 cache are more bigger.

If I remenber correctly arcum change some inline in others place. But I will check it. Thanks for the return.
It fix the problem except for MulShr32 function. However the issue go away when I declare static the function !!! The function is only used on the mixer file so I think you can do it.
Yup, that's fine. Smile
Hi, here an others set.

+ inline fix: I take the hint of gigaherz. Duplicate the clamp_mix function that cause the failure. With the keyword static gcc is happy and do the inline. However I found an interesting behavior, clamp_mix function (before & after patches) are not inlined in Reverb.cpp. I try to also declare the clamp function (on reverb.cpp) static but it causes some inline error in others places... Opinion are welcome.

+ gamefixes_like_speedhach: I join a screenshot of the new interface. I move the check box at the top and change heading message. It is now more similar to speedhack panels.

+ gs log api: I update zzogl plugins to use the GSsetLogDir API. Note I also change the name of the log file to avoid some collision when you using an others gs plugins.

Feedback are welcome.
Pages: 1 2 3 4 5