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

No longer use asm-processor #1824

Merged
merged 25 commits into from
Mar 1, 2024
Merged

No longer use asm-processor #1824

merged 25 commits into from
Mar 1, 2024

Conversation

Dragorn421
Copy link
Collaborator

Speeds up builds noticeably. time make RUN_CC_CHECK=0 is 20-25% faster for me

asm-processor is still there ready to be used if we ever have more GLOBAL_ASMs to use

Addresses #1719

Changes:

#ifdef __GNUC__
#define CS_FLOAT(ieee754bin, f) (f)
#define CMD_F(a) {.f = (a)}
#else
#define CS_FLOAT(ieee754bin, f) (ieee754bin)
#define CMD_F(a) {(a)}
#endif
  • update csdis.py
  • add csdis_re.py to reextract committed CutsceneData (using csdis.py)
  • add reencode.sh to handle writing EUC-JP source for ido to process (could be done in the makefile directly but would look quite ugly. it is unfortunate ido cannot read source from stdin)
  • add bss padding in z_collision_check.c (doesn't match without)

Comment on lines +311 to +313
# For using asm_processor on some files:
#$(BUILD_DIR)/.../%.o: CC := $(PYTHON) tools/asm_processor/build.py $(CC) -- $(AS) $(ASFLAGS) --

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment necessary? Hopefully people should not need to do this, and if someone was seriously thinking about re-introducing asm processor they could probably figure out how to invoke it (say by looking at git history)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't touch DEP_FILES which is part of asmproc Makefile logic, I don't want to completely pluck it out so I'd rather document explicitly asm_processor can be used

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, why don't you want to pluck out the asmproc.d stuff too? Are you expecting people to re-enable asm_processor locally only for dependency tracking?

I'm not sure I like the idea of only "half-heartedly" removing asm-processor, ideally we would delete it from the repo entirely to try to make things simpler. I personally don't think we'll need GLOBAL_ASM ever again, but if that's still an issue, maybe it's better to wait on this change than making the build system more complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is a noticeable performance boost for IDO builds so I don't want to post-pone it

I do think we could see GLOBAL_ASMs again in the repo, for example if/when I figure out how to build matching in a non-intrusive way maybe a single function in a file would resist matching and would be global_asm'd

maybe I refactor now which files to run asm-processor on? i.e. ASM_PROCESSOR_O_FILES :=

Comment on lines 24 to 26
#ifdef __GNUC__
#define CS_FLOAT(ieee754bin, f) (f)
#define CMD_F(a) {.f = (a)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the biggest fan of this macro taking two numbers, its pretty confusing to see two numbers being passed into a macro and only one to be used depending on the context. I tbh would prefer if hex was used in all cases with comments to describe the float representation

I guess this is a solution to the "portability" problem that was mentioned before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we have the option to just write the hex as well rather than the macro

Personally I don't mind either

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I mean, I'll take this over what we have now, and if it will be a compromise for those who want float representation to be used sometimes. But I don't think its ideal

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to "hex"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do the zapd cutscene files have the float representation as a comment? I see the non-zapd ones dont. feel like that is necessary at least for the the cam commands where the floats need to be intelligible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't
I can probably PR another "hex-comment" option to ZAPD, sounds simple enough for me to do

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright
I feel like we may have not had enough input on this-- I like just hex, but the macro with dual input might be a good compromise for those who feel strongly about them being in float form

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR I don't mind changing back

It's not particularly difficult either:

  • the macro definitions
  • the zapd argument in extract_assets.py
  • the similar hardcoded argument in csdis.py
  • run csdis_re.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed back to "both" (CS_FLOAT(hex, f) macro) after having too much trouble trying to have a tool encode floats on build like asm-processor did

@Dragorn421
Copy link
Collaborator Author

Ready for more review

@fig02 fig02 merged commit bdee3d3 into zeldaret:main Mar 1, 2024
1 check passed
@Dragorn421 Dragorn421 deleted the meta_no_asmproc branch March 1, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants