Skip to content
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

GRAPHICS: Make HQx ASM scalers relocations free #3636

Merged
merged 2 commits into from Jan 3, 2022

Conversation

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jan 1, 2022

This is considered as a good practice to not have relocations in text segment.
The resulting binary is now completely PIC and can easily be relocated at any address (ASLR).

After a quick benchmark done on real game, the performance seems to be equivalent if not faster to the older assembly code.
The C++ implementation is still around 3 times slower even in x86_64 mode (so with SSE2 extensions enabled).

This is considered as a good practice to not have relocations in text
segment.
The binary is now completely PIC and can easily be relocated at any
address (ASLR).
@ccawley2011
Copy link
Member

@ccawley2011 ccawley2011 commented Jan 1, 2022

Would it be possible to move the static variables hqx_params and RGBtoYUV into the HQScaler class? The HQx scalers are currently the only ones that don't support having multiple instances following PR #3557.

@lephilousophe
Copy link
Member Author

@lephilousophe lephilousophe commented Jan 1, 2022

@ccawley2011 Do you mean like this? 😉

@aquadran
Copy link
Member

@aquadran aquadran commented Jan 1, 2022

it would be nice to add 64bits calling convention in future

@lephilousophe
Copy link
Member Author

@lephilousophe lephilousophe commented Jan 1, 2022

it would be nice to add 64bits calling convention in future
The whole asm code is using 32 bits registers.
I would say that the code should be rewritten with x86_64 registers in mind and with SSE2 instructions in another file.
This may improve performance.

@aquadran
Copy link
Member

@aquadran aquadran commented Jan 1, 2022

I'm not sure if it's worthy rewrite it, I mean, not sure what improvements would be. I'm just saying use it what we have at least. I guess some tunings might be required, like access to memory where it's 32bits

graphics/scaler/hq.cpp Outdated Show resolved Hide resolved
@bluegr
Copy link
Member

@bluegr bluegr commented Jan 2, 2022

Nice work making the code PIC (Position-independent)!

This allows multiple instances to run in parallel
@lephilousophe
Copy link
Member Author

@lephilousophe lephilousophe commented Jan 3, 2022

Merging now

@lephilousophe lephilousophe merged commit 5f86c39 into scummvm:master Jan 3, 2022
8 checks passed
@lephilousophe lephilousophe deleted the pic-hq branch Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants