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

New 816 opt #184

Merged
merged 11 commits into from Apr 8, 2023
Merged

New 816 opt #184

merged 11 commits into from Apr 8, 2023

Conversation

kobenairb
Copy link
Contributor

@kobenairb kobenairb commented Apr 4, 2023

Hello everyone,

A PR to decommission the tcc optimizer in Python in favor of C.
I added that in tools (now 816-opt and constify can be found in devkitsnes/tools/) and refactored the makefiles a bit. I updated the readme of the project and the wiki pages.

Note: I could only test on Linux, feedback on win and mac would be a plus. My repository is also located here https://github.com/kobenairb/opt-65816. I tested idempotence between the Python and C version and memory leak with Valgrind.

The goal is to abstain as much as possible from dependencies (python being only used for this case), and plan to include it in the near future (more or less ^^) directly in tinycc, and do the same for constify ( migration to C in progress).

Plus a few other small changes:

  • Dockerfiles update
  • Update docker/md5/ file

I tested each roms (snes-examples) as well as launched all docker containers, everything seems OK.

Finally (nothing to do with this PR), I had fun pointing the WLA to the last commit (5f225addb800b6913ae3212fb2f5c4d29b0ae7e5), and tcc to the tinycc-65816-0.9.26 branch, everything seems ok too. We could also switch all that to these latest versions. For tcc, we would finally have 64bit that works for everyone. To discuss together.

Well, keep me posted. Any comments are welcome :)

@RetroAntho
Copy link
Collaborator

Hi kobenairb,

Thanks for this PR ! We will review it in the coming days/weeks as it contains a lot of changes and will probably require minor updates.

For coming PR, may i ask you to split it in smaller PR (one with the tools, one for docker, one for wiki and so on...) ?
It will be more easy for us to follow and merge.

Few remarks quickly :

  • your removed the 2 spaces at the end of "Special thanks" category in the readme.md but it is needed.
    As it is markdown syntax, the 2 spaces at the end allow to return to the line, without jumping a line !
    I saw some formatting update in wiki pages, id not not know if it can impact the final result.

You updated some install rules to remove the "mkdir -p" instruction but in some cases, the rule will failed as the folder may not exists (if we create a new branch, the bin or tools directories will not exists as git keep only folder containing something)

See you soon :)

@kobenairb
Copy link
Contributor Author

Hello @RetroAntho,

Thanks for your return :)

I try to answer your questions one by one rather than writing a novel.

  • "For coming PR, may I ask you to split it in smaller PR...": Yes Sir, I'll do it for the next big PR. I thought about it but I found it odd to make several PRs for the same subject: Indeed, here everything is linked (with a few exceptions) to the migration of the Python version of 816-tcc towards that in C:

    • tools/: I stored the new version in C to do like bin2txt, snesbrr...
    • docker/: I modified the Makefile to remove Python dependencies
    • wiki/: I deleted all the Python part: installation, FAQ...

Where I should have cut (one PR per change), but I'm very lazy:

  • The modification of snesbrr: I added the clean of the .d files.
  • The Update of the md5 file used by docker to check ROMS. I needed it to validate the 4 types of Linux container.
  • The modification of the Makefile in tools/**/: here I could have made the mkdir in the Makefile of 816-opt as for the others at first, then another PR to delete them.
  • The modification of the sed in 2 Makefile: I added the -i (in place) and removed the step from the temporary file.
  • The move of constify from bin/ to tools/. Here I tried to harmonize a little. Knowing that eventually constify and 816-opt will be managed by tcc itself.

Otherwise,

  • "your removed the 2 spaces at the end of "Special thanks...": My bad, it's a typo on my part, you're right, I'll correct that quickly in the PR.

  • "You updated some install rules to remove the "mkdir -p the rule will failed as the folder may not exist (if we create a new branch, the bin or tools directories will not exist as git keep only folder containing something)": We will have no problem, the trick is in bin/.keepme and tools/.keepme. By doing this, git keeps the directories because they are never empty (presence of .keepme). I found it easier because we ask git to manage the tree structure of the project rather than having to add this in each Makefile (especially since we have a lot of them). the structure of the project. Tell me if I am clear ^^

    In any case, thank you for the time granted and your quick response.

    I do the fix for the Readme quickly and go back to the tcc migration...

@RetroAntho RetroAntho merged commit 48ebc59 into alekmaul:develop Apr 8, 2023
@RetroAntho
Copy link
Collaborator

Hi,
I did tests, everything seems to be ok and we can merge it.
Just saw that the bin and tools folders under devkitsnes are not created anymore, i will add the .keepme file to be consistant.

Thanks for your work !

@kobenairb kobenairb deleted the new_816-opt branch April 13, 2023 01:33
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.

None yet

2 participants