Skip to content

Adding SDL_Assume macro to give hint to the optimiser #6309

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Oct 1, 2022

with a given condition generating less instructions.

@sezero
Copy link
Contributor

sezero commented Oct 1, 2022

Gcc case: __builtin_unreachable requires gcc >= 4.5, and since you add
the new macro to a public header then please stricten the condition like:
#elif defined(__clang__) || (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ > 4)))
I don't know since when clang supports __builtin_unreachable - the only
version I have is clang-3.4.2 and it just does...

Msvc case: As I remember, __assume() became useful only since VS 2005:
https://stackoverflow.com/questions/3096939/ (and no, I can not remember
since which version it has been supported.) So, I'd avdise strictening
its condition, like: #if defined(_MSC_VER) && (_MSC_VER >= 1400)

@devnexen
Copy link
Contributor Author

devnexen commented Oct 1, 2022

oh I did not know you supported sdk so far back...

@sezero
Copy link
Contributor

sezero commented Oct 1, 2022

oh I did not know you supported sdk so far back...

Well, my every-day machine runs on an old CentOS-6.10 and it comes with gcc 4.4.7.
I'm one of those said-to-be-extinct dinosaurs :)

@slouken slouken added this to the 3.0 milestone Nov 11, 2022
@icculus
Copy link
Collaborator

icculus commented Nov 23, 2022

A silly question from me: can we not just add the assume keywords to SDL_assert itself? Clang's static analyzer already uses SDL_assert as an assumption that the asserted case must be true (that the analyzer_noreturn part in SDL_assert.h).

@icculus
Copy link
Collaborator

icculus commented Nov 23, 2022

(Err...and add some preprocessor magic to keep the assumptions when assertions are disabled, to be clear.)

@slouken
Copy link
Collaborator

slouken commented Mar 10, 2024

A silly question from me: can we not just add the assume keywords to SDL_assert itself? Clang's static analyzer already uses SDL_assert as an assumption that the asserted case must be true (that the analyzer_noreturn part in SDL_assert.h).

This seems like a good idea.

with a given condition generating less instructions.
@icculus icculus self-assigned this Mar 10, 2024
@devnexen
Copy link
Contributor Author

Ah ignore my last commit I did not see @icculus wanted to take over.

@icculus
Copy link
Collaborator

icculus commented Mar 10, 2024

You can keep going, I haven't looked at this in a year, and just want to make sure I pay attention to it soon. :)

@devnexen
Copy link
Contributor Author

devnexen commented Mar 10, 2024

The thing is , looking at it now, I m not sure we can just use SDL_assume like SDL_assert ... on clang.

@slouken
Copy link
Collaborator

slouken commented Aug 6, 2024

Do we have any benchmarks showing that this provides any benefit? I'd hate to add something to the API that people will start using to improve their code if it doesn't actually help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants