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

Yet another disasm.py speedup #918

Merged
merged 28 commits into from
Jul 12, 2022
Merged

Conversation

AngheloAlf
Copy link
Collaborator

Change disasm.py to use rabbitizer as the instruction decoder instead of the bundled mips_isa.py file.

The advantage of using this package instead of what we are doing now is this package is a fast MIPS instruction decoder fully written in C, so runtime and memory usage are greatly improved.
For example, running the disasm.py script on current master:

$ time python3 tools/disasm/disasm.py -j 1 --reg-names=o32 --all

...

real    1m1.855s
user    1m1.200s
sys    0m4.860s

This PR:

$ time python3 tools/disasm/disasm.py -j 1 --reg-names=o32 --all

...

real    0m32.606s
user    0m30.492s
sys    0m4.293s

(I couldn't get proper memory usage measurements because the multithreading stuff messed up with them. lmk if anybody knows a proper way to measure it)

I cleaned up and de-duplicated some code. It is a bit more readable imo.

I also took this opportunity to clean up the requirements.txt file and remove what seems to be dead dependencies and add diff.py dependencies and rabbitizer

Copy link
Collaborator

@hensldm hensldm left a comment

Choose a reason for hiding this comment

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

I'm also checking it out to do some testing

tools/disasm/disasm.py Outdated Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
Co-authored-by: Derek Hensley <[email protected]>
Copy link
Collaborator

@hensldm hensldm left a comment

Choose a reason for hiding this comment

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

Seems to work fine for me. Always enjoy the speed ups :) .

Copy link
Contributor

@EllipticEllipsis EllipticEllipsis left a comment

Choose a reason for hiding this comment

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

Works on Mac, code looks good from what I can see, but there's a few issues with the surroundings.

steps {
echo 'Installing Python dependencies'
sh 'python3 -m pip install -r requirements.txt'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we could just subrepo it instead, this basically counters all new timesave.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The package is written in C, so people would require to build it themselves. They would require to install the python dev package to build it too. People on Windows (cygwin, not wsl) would require the VisualStudio toolchain just to build it. Too much trouble imo.

Also, this step only adds 2 extra seconds on the jenkins side

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah subrepo doesn't really help since Jenkins would have to build it anyways, which is almost certainly going to be longer than just installing a few python packages.

Comment on lines 44 to 46

# Floating-point status register
.set $FpcCsr, $31
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a subrepo, right? So this should be PR'd to that repo. And the permuter might need it too. And maybe decomp.me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. As every time this topic comes up, the asm processor is not a subrepo... for reasons I can never remember.

If using this register alias is an issue I can disable it on rabbitizer's configuration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

@EllipticEllipsis EllipticEllipsis Jul 9, 2022

Choose a reason for hiding this comment

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

I think we can probably live without this, I expect mips2c m2c wouldn't like it either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the use of $FpcCsr for now instead

@AngheloAlf AngheloAlf mentioned this pull request Jul 9, 2022
@Kenix3 Kenix3 added Merge-ready All reviewers satisfied, just waiting for CI and removed Last-review labels Jul 11, 2022
@AngheloAlf AngheloAlf merged commit 54957f8 into zeldaret:master Jul 12, 2022
@EllipticEllipsis EllipticEllipsis removed the Merge-ready All reviewers satisfied, just waiting for CI label Jul 12, 2022
@AngheloAlf AngheloAlf deleted the rabbitizer branch July 12, 2022 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants