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

Have pokeemerald use OFast, aapcs, and ftoplevel-reorder for MODERN by default #1945

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

Conversation

AreaZeroArven
Copy link

The big question is: why make these changes?

Let's take a look:
OFast is a compiler flag that allows for some standard-breaking optimizations, mostly in the floating point category. Pokeemerald rarely uses floating points and where it does, it does not require extreme accuracy. In addition, OFast optimizes for speed, not space, of which there is always 32 MB of anyway.

AAPCS is the ABI that supports arm7tdmi chips and up. It mostly mandated how functions start and end in order to interwork with thumb code. apcs-gnu was back when pokeemerald was still being decompiled.

Finally, ftoplevel-reorder, it is not able to be disabled on clang, and without disabling top-level reordering, the game does not boot. Or rather, it doesn't boot because only one file, m4a.c, does not play nicely with the compiler, causing the game to crash when initializing the sound. For this reason, m4a.c gets its own exception in the Makefile.

These changes should be beneficial to anyone compiling on MODERN and there does not seem to be any downsides. These changes were thoroughly tested for weeks on my end, to ensure no side effects happened. I hope you enjoy this!

Signed-off-by: Arven

…y default

The big question is: why make these changes?

Let's take a look:
OFast is a compiler flag that allows for some standard-breaking optimizations, mostly in the floating point category. Pokeemerald rarely uses floating points and where it does, it does not require extreme accuracy. In addition, OFast optimizes for speed, not space, of which there is always 32 MB of anyway.

AAPCS is the ABI that supports arm7tdmi chips and up. It mostly mandated how functions start and end in order to interwork with thumb code. apcs-gnu was back when pokeemerald was still being decompiled.

Finally, ftoplevel-reorder, it is not able to be disabled on clang, and without disabling top-level reordering, the game does not boot. Or rather, it doesn't boot because only one file, m4a.c, does not play nicely with the compiler, causing the game to crash when initializing the sound. For this reason, m4a.c gets its own exception in the Makefile.

These changes should be beneficial to anyone compiling on MODERN and there does not seem to be any downsides. These changes were thoroughly tested for weeks on my end, to ensure no side effects happened. I hope you enjoy this!

Signed-off-by: Arven
@AreaZeroArven AreaZeroArven changed the title Have Pokeemerald use OFast, aapcs, and ftoplevel-reorder for MODERN by default Have pokeemerald use OFast, aapcs, and ftoplevel-reorder for MODERN by default Nov 3, 2023
This file also relies on function ordering to operate properly, even though as of this moment, GCC happens to not do that.
@luckytyphlosion
Copy link
Member

We cannot assume that people are okay with non-standard floating point optimizations. If users believe that they need the extra speed, then they can enable it themselves. Also, -Ofast implies loop unrolling, which might produce larger code (whether that matters is another thing).

I'm not fully sure on the history of apcs-gnu vs aapcs.

fno-toplevel-reorder mainly exists for the case where the game does UB with adjacency of variables. The example I thought of (saveblock ASLR) appears to have been fixed, but there may be other instances available. Also, some people may prefer functions and variables to be in the order that they are defined. I'd imagine that the majority of people would prefer that behaviour since it is deterministic and knowable (although the the modern loadscript doesn't have a deterministic object order anyway so maybe that is precedence against fno-toplevel-reorder, but perhaps a nondeterministic object order was a mistake anyway).

@luckytyphlosion
Copy link
Member

@PikalaxALT says that apcs-gnu is required to link with agbcc's libgcc, but I'm not sure if current pokeemerald uses agbcc libraries anyway (I don't think so but didn't do thorough investigation)

@luckytyphlosion
Copy link
Member

I predict that this PR will rot because nobody here knows enough about modern gcc stuff to accurately determine whether these changes will break anything.

@SBird1337
Copy link
Collaborator

I was genuinely interested in this, partly at least because -fno-toplevel-reorder seems kind of useless. fwiw it seems like the author did not test the changes because it just crashes the game on boot.

About aapcs: We probably don't want to do that, because it imposes a double-word stack alignment that is very non-beneficial for the arm7tdmi.

About -Ofast I think it was already discussed, but -ffast-math is not really something you'd want. Additionally this generates quite aggressive loop unrollings due to O3.

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.

3 participants