diff --git a/AGENTS.md b/AGENTS.md index 931b226b3..a52f71e8f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,8 @@ The ultimate goal of the project is not just a matching binary, but **human-read ## Repository Layout +Paths below use `GMSJ01`, the default version. Each supported version has the same layout under its own name. + ``` config/GMSJ01/ config.yml — decomp-toolkit project config @@ -63,8 +65,33 @@ Ninja is used for most workflows. Simply running `ninja` in the root of the repo To check changes against a baseline, `ninja baseline` can be used to generate a baseline report on the current state of the work tree, after which `ninja changes_all` can be used to show how the current changes to work tree influence matching progress, including a per-symbol report. **Important**: for regression testing, you **must** use `ninja baseline` and `ninja changes_all`. -If the worktree already has changes and you want to test for regressions when compared to master, stash the changes, call `ninja baseline`, pop the stash and call `ninja changes_all`. When asked to work on a task, if a worktree is clean or only contains general scaffolding like classes and function stubs, you **must** use `baseline` before starting work and `changes_all` after finishing work to report on the results to the user. +If the worktree already has changes and you need a baseline of the committed tree, ask the human to stash and restore the changes. + +**Never** use `git checkout`, `git restore`, `git stash`, `git reset` or `git checkout-index` to undo an experiment or to get a "before" state. +The worktree usually holds the human's uncommitted work and your earlier changes in the same files, and these commands destroy all of it. +To measure "before" and "after", copy the files to a scratch directory, make the change, measure, and copy the files back. +Or undo the change with an edit. + +The project builds five versions of the game (see `README.md`): `GMSJ01`, `GMSP01`, `GMSE01`, `GMSJ01_01` and `GMSP01_NX`. +Before you commit a change, test **all** of them for regressions, not only JP. +Each version keeps its own baseline in `build//baseline.json`, so you can make all the baselines first and compare all of them after the change: + +```sh +# before the change +for v in GMSJ01 GMSP01 GMSE01 GMSJ01_01 GMSP01_NX; do python configure.py -v $v && ninja baseline; done +# after the change +for v in GMSJ01 GMSP01 GMSE01 GMSJ01_01 GMSP01_NX; do python configure.py -v $v && ninja changes_all; done +python configure.py # go back to the default version (GMSJ01) +``` + +Each version must also still report its DOL as `OK`. +A version whose disc image is not in `orig//` cannot be built. Tell the human which versions you could not test. +`objdiff.json` and `decomp-diff.py` follow the last `configure.py -v`, so reconfigure to the version that you want to diff. +Do not build old commits in separate trees. + +When a sweep changes many sites (for example an accessor at every read of a field), report the number of functions that got better, the number that got worse, and the number that reached or lost 100%. +Read the full `changes_all` report, not only the function you work on: a frame gain in one function is not a match if another function gets worse. The underlying utility used for splitting the binary is `dtk` (`build/tools/dtk`). It is already properly set up and almost never requires any meddling. @@ -219,6 +246,36 @@ Important limitations: - `m2c` does **not** support the C++ context flow needed for this repository's real types, so do not try to feed it SMS C++ context files and expect good typed output. - use `m2c` for orientation only; never trust it over the asm or decomp-diff. +### Read the compiler instead of guessing: the MWCC decompilation + +A 100% matching decompilation of `mwcceppc.exe` exists: [`rayanht/mwcc`](https://github.com/rayanht/mwcc). +It covers GC 1.0, 1.1, 1.1p1, 1.2.5, 1.2.5n and 1.3, so it includes the exact compiler that SMS uses (`GC/1.2.5`, and `GC/1.2.5n` for a few TUs). +Usually it is at `../mwcc` relative to the project root. + +Some problems do not yield to guesswork, for example: + +- a register swap or rotation when every instruction already matches, +- a frame that is a few slots too big or too small, +- an inline that the ROM expands at one site and calls at another, +- a CSE, a copy or a load that MWCC keeps or removes for no visible reason, +- a symbol in the wrong section, or a weak symbol in the wrong TU. + +For such problems, reading what the compiler does is much faster than trying source spellings one by one. +When you meet such a problem and the decompilation is not available, **stop and ask the human to clone it**, and tell them the path where you expect it. +Do not continue to guess spellings for hours when you could read the answer. + +Where to look: + +| Topic | Files | +|---|---| +| Parser, expressions, inlining, templates, mangling | `src/frontend/` (`CInline.c`, `CExpr.c`, `CDecl.c`, `CTemplate*.c`, `CMangler.c`) | +| IR optimizer: copy propagation, CSE, struct scalarization | `src/optimizer/Iro*.c` | +| Code generation, register colouring, scheduling, stack frame, peephole | `src/backend/` (`CodeGen.c`, `Coloring.c`, `InterferenceGraph.c`, `StackFrameEABI.c`, `Peephole.c`) | + +Use the decompilation to **explain** a codegen difference, and then find a source spelling that a human would write. +An explanation of the compiler is not permission for a fakematch: the rules in [Fakematches](#fakematches-important) still apply. +Check each conclusion with a real build of SMS. [docs/AGENT_MATCHING_TIPS.md](docs/AGENT_MATCHING_TIPS.md#use-the-compiler-sources-and-the-mwcc-debugger) has the facts that were already read from it, and the `mwcc-izer` debugger that dumps the compiler's internal state for one function. + ### Decompiling brand-new code without existing scaffolding When a user requests to decompile a fresh TU for which no stubs or header exists, `docs/PROGRAM_STRUCTURE_REVVING.md` **must** be followed to reconstruct the scaffolding like declarations/definitions of methods/functions, class definitions, global variables, etc. @@ -226,6 +283,64 @@ When a user requests to decompile a fresh TU for which no stubs or header exists **Important**: as soon as you notice some method declaration and/or definition missing, thats a sure-sign of someone forgetting to scan the entire `mario.MAP` file properly for all known methods of some class and/or all known symbols in a TU. Do the rescan yourself for relevant classes and TUs using relevant regular expressions containing file name or the correct mangling suffix that all such class members have. +## Evidence + +Every name, every helper and every body in the source needs evidence. +A guess that compiles to the right bytes is not evidence: many different sources compile to the same bytes. + +### What counts as evidence + +In order of strength: + +1. A name in `mario.MAP`: a function, a data symbol, or a class name inside a mangled signature. +2. A debug string in the binary, for example the field names that `PARAM_INIT` stringifies, or an assert condition. +3. A setter that stores the field, especially one that also configures something else from the same value. +4. An allocation size: `count * sizeof(T)` names the array that it fills. +5. The parameter name of the function that the value goes to. +6. The size of an UNUSED map symbol. + +A name in **our own** source is not evidence. +A local variable name, a placeholder or a field name that someone wrote earlier (often an agent in the same session) is a guess. +Do not read it back as proof. + +### The maps + +- `orig/GMSJ01/files/mario.MAP` is the main source of truth. +- `config/GMSJ01/symbols.txt` lists only the symbols that survived in the DOL. + A function that is missing there can still be an UNUSED map entry with a size. + Before you say that a function is inline, stripped or missing, grep `mario.MAP` for its mangled name. +- The symbol-closure part of `mario.MAP` (the tree before `.init section layout`) has a `>>>` line for each other TU that emitted the same weak symbol. + The closure, not the section layout, shows which TUs emitted an inline. + A weak symbol that the map lists but our build does not emit shows a site where MWCC did not inline, in the ROM. +- `orig/GMSJ01/files/marioL.MAP` is the map of a **different** link that keeps the `JAW*` debug windows. + About 287 functions that `mario.MAP` calls UNUSED are linked there. + In its closure tree, the entries indented one level below a function are the symbols that the function references: its call list. + Read it before you write any UNUSED body. +- Other versions have their own maps, for example `orig/GMSP01/files/marioEU.MAP` (PAL) and `orig/GMSJ01_01/files/mario.MAP` (JP Rev 1). They can name functions that the JP map does not. +- A function that another version's map names but the JP map does not is usually the same source, dead-stripped from the JP link. + Write the body without `#ifdef` in the shared `.cpp`. The JP DOL stays identical, because the linker removes it again. + Confirm this with a build of every version. + +### Other games + +Other decomp projects share JSystem and some game-layer code. +Each one uses a **different MWCC version**, so take only source-level facts from them: names, field order, control flow, whether a function or an assert existed. +Never use their frame sizes, instruction counts or register assignments as evidence for our build. + +| Project | Symbol evidence | +|---|---| +| The Wind Waker (TWW) | A **debug** map with demangled names. It lists every inline that each TU called. Asserts are on, so its string pools keep assert conditions of dead-stripped code. | +| Twilight Princess (TP) | Retail and debug builds. Good for JSystem names and asserts. | +| Mario Kart: Double Dash (MKDD) | A review build that ships a linker map with mangled names only. It is **not** a debug build. | +| Animal Crossing | Shipped linker maps (`static.map`, `foresta.map`) with function and file names. Its decomp has the older C version of the JAudio library (`jaudio_NES`), a close relative of SMS's JASystem. Its function bodies are decompiled, and their names are evidence only where the AC maps confirm them. | +| Pikmin 2 | Some `JAInter` field names. | +| Luigi's Mansion | **No** map. Its names are the decompilers' own. Never cite it. | + +A decomp project's `asm` and `symbols.txt` files are outputs of naming decisions, not evidence. +Trace a borrowed name back to a shipped map or to strings in that game's binary. +Check every borrowed name against a use in SMS: TWW is sometimes wrong for the SMS revision. +SMS's own map wins over any other game on names and on inline-or-not. + ## Source Organization Each `.o` file maps 1:1 to a `.cpp` file. The path is listed in `configure.py` under `config.libs`. @@ -343,13 +458,23 @@ Game-tunable parameters use `TParamRT` fields initialized via the `PARAM_INIT ### Comments & annotations in the code - `// fabricated` — the code is not from the original binary; it was invented/guessed to make things compile. May be incorrect. -- `// TODO:` — known issues, suspected inaccuracies, or incomplete understanding. -- `// Tiny size mismatch` / `// TODO: incorrect size` — for functions which were completely inlined and not emitted to the original binary, but are present in the symbol map as UNUSED symbols and their size is available. That size can be compared against the size in our code using tools/decomp-diff.py and if it's different, then the guess for the function's contents is not correct yet. +- `// TODO:` — known issues, suspected inaccuracies, or incomplete understanding, for example a guessed name. - `// correct but X is incorrect` — the function itself matches, but a called function does not. - `// rogue includes needed for matching sinit & bss` — includes added purely for BSS/static-init ordering. -- `#pragma dont_inline on/off` — forces the compiler to not inline a function (required for matching in specific cases). +- `#pragma dont_inline on/off` — forces the compiler to not inline a function. This always hides a real cause (see the inlining section of the tips document). +- `/* 0x1EC */` — the offset comment of a field. It is part of the layout record. -Don't be afraid to leave notes that would be useful to the next person trying to match the code, figure it out, or in the far future, write mods for the game. +Keep comments to these short markers. +Do not write explanatory prose in source files: no paragraphs above a declaration, no notes after a field, no text that summarizes a function. +A header is a layout record: write the offset comment and the declaration. +Put the reasoning and the evidence in the PR description or in the commit message. +Write prose in source only when a human asks for documentation. + +Do not write a `// TODO` about a frame deficit or about the size of an UNUSED body. +Such notes go out of date at once, and `decomp-diff.py` shows the difference anyway. +Write "frame-only" in the PR description. + +Keep existing `TODO` comments unless your change resolves them, and delete a `TODO` that is no longer true. ### Style @@ -357,6 +482,15 @@ Don't be afraid to leave notes that would be useful to the next person trying to - C++98 style: no auto, no range-for, no lambdas, no `nullptr` keyword (it's a macro). - `#include` paths use angle brackets with project-relative paths: `#include `. - Header guards use `#ifndef FOLDER_FILENAME_HPP` / `#define` / `#endif` pattern. +- When the bodies of a class template must leave the header, put them in `
.tpp` (never `
Impl.hpp`), with a `_TPP` include guard. + The `.tpp` includes its own `.hpp` first. + Include the `.tpp` only from the TUs that the map shows as owners of the template's weak symbols. +- Cast between a pointer and an integer with `uintptr_t`, never with `u32`, `s32` or `int`. + On PowerPC this changes no byte, and it keeps a 64-bit port possible. + Do not change casts that do not involve pointers. +- Port the asserts that a reference project supports, as `JUT_ASSERT();` with no line number. + A disabled assert still changes codegen. +- Write a release-stripped warning with the existing `JUT_REPORT_MSG(...)` or `JUT_ASSERT(...)` macros, never with an empty `if (cond) { }`. - When working with dolphin's mtx.h library, always prefer routing calls to PSMTX* or PSVEC* functions through the non-PS-prefixed macros MTX* and VEC*. This is so that when a debug build of the game is eventually built, we still can easily switch between paired-singles PowerPC assembly implementation or a regular C implementation (C_MTX* and C_VEC*). However, sometimes the game code directly calls C_ prefixed versions of the functions, in which case we should too. @@ -378,6 +512,48 @@ It must however be noted that such tricks don't always resolve stack frame issue If only stack offsets still differ, stop. Leave the function nonmatching and write "frame-only" in the PR description. Don't commit locals, casts or temporaries whose only job is the frame. +#### Frame deficits are not evidence + +A frame deficit can be closed in many ways. +Two `f32` pads, two `s32` pads, one `f64`, four `u16`s or two pointers all give the same 100%. +An invented accessor almost always gives the missing slots too. +So a frame match alone says nothing about what the original wrote. + +These are fakematches, and reviewers reject them on sight: + +- an unused local that only adds a slot, +- a named or reference local inside a one-line accessor, only to add a temp: + `u8 getGlobalAlpha() const { const GXColor& c = mGlobalPrmColor; return c.a; }`, +- a getter for a class's **own** field, called inside that class's own methods. + A method of the class reads its own fields raw. + An accessor on **another** object is normal. +- an accessor that the frame size alone chose. + If a deficit could equally come from an accessor for `unk61`, `unk60` or `unk4C`, the frame does not tell you which one. + +The frame measurements in [docs/AGENT_MATCHING_TIPS.md](docs/AGENT_MATCHING_TIPS.md#the-stack-frame) are **diagnostics**. +Use them to measure how many slots are missing and where. +Commit a spelling only when something outside the frame also supports it: an instruction difference, a sibling function in the same file that uses the same idiom, a reference project, a debug string, or the map. + +Remove every pad and probe from a function before you measure anything else in it. +A pad sets the frame, so a correct candidate competes with it and measures worse than it is. + +#### The accessor consistency test + +An accessor that has no map symbol is a hypothesis. +Test it at **every** site in the game that touches that member, then read the whole `changes_all` report. + +- A real accessor holds everywhere: no function gets worse. +- If any function gets worse, the hypothesis is refuted. Report it as refuted and remove it. +- Do not keep the accessor at the sites where it helps and remove it where it hurts. That is choosing by score, and it hides the evidence. +- An accessor that changes nothing anywhere has no evidence either. + +A helper that the ROM **proves** (its out-of-line copy matches byte for byte, or the map names it) is different. +Use it at every site whose code it explains, also where the match percentage drops. +A site that gets worse then shows other damage in that function; do not hide it with a hand-written block that scores better by chance. + +A few accessors are known exceptions, where the original used the getter and the raw field without a rule. +They are listed in [docs/AGENT_MATCHING_TIPS.md](docs/AGENT_MATCHING_TIPS.md#known-accessor-verdicts) with the other sweep results. + ## UNUSED Functions (Inlined/Dead Code) This is a critical concept for matching decomps. @@ -410,6 +586,47 @@ UNUSED functions must still be reconstructed in the source because: 4. **Some UNUSED functions are truly dead**: If no code in the TU resembles the function, it may have been unused or called only from other UNUSED functions. +### Decide fast whether an UNUSED body is recoverable + +An UNUSED function defined out of line in a `.cpp` can only be inlined **inside its own TU**: other TUs see only the declaration. +UNUSED also means that no linked function calls it out of line. +So there are two cases, and one command tells them apart: + +``` +python tools/decomp-diff.py -u -s nonmatching -t function +``` + +- A linked function in the TU is missing a block of about the UNUSED size: that is the inline site, and its ROM assembly **is** the body. Recover it. + The caller's match rises, and the standalone copy hits the map size: two checks. +- Every linked function in the TU already matches: nothing in the shipped game reaches the UNUSED function. Leave the stub empty. + +Before you call a body unrecoverable, also check `marioL.MAP` (see [The maps](#the-maps)) and the reference projects. + +### Never fabricate an UNUSED body + +Write a body for an UNUSED symbol only when you can reconstruct it from evidence: an inline site in the same TU, a function in `marioL.MAP`'s call tree, or a reference project. +With no evidence, leave the stub empty (`void Foo::bar() { }`), and write no `TODO`. +`validate-symbol-order.py` reports the size difference as a warning, and warnings never fail. + +An exact size is a **necessary** check on a body that came from evidence. +It is never a **reason** to believe a body that you guessed: many different bodies compile to the same size. +Do not tune a guessed body until its size matches. + +Stronger than the size is the **weak helper cluster**. +MWCC emits the weak helpers that a function first needs immediately after that function, so the run of weak symbols after an UNUSED entry in the map is a fingerprint of its body. + +- A `__dt__` in the run means a by-value local of that type. +- `__construct_new_array` means `new T[n]`. +- A run of one class's accessors means that the body touched that class. + +Check that your body brings back the whole run in the map's order, not only its own size. +Do not declare such a destructor yourself: let the call site generate it. + +Before you size a destructor body, subtract the delete tail: an empty destructor is already about 0x70 bytes (see the tips document). + +A fabricated helper that has no UNUSED entry must be `inline` (or `static inline`), not plain `static`. +A plain `static` function stays in the object after MWCC expands all its sites, so the map would list it as UNUSED. + ## Working with the Codebase ### Key tasks an agent may help with @@ -427,17 +644,30 @@ UNUSED functions must still be reconstructed in the source because: - **Always verify**. All decompiled code must be verified against the ground truth (original binary) via `decomp-diff.py`. - **Always use tools for arithmetic**. When computing sizes, offsets, instruction counts, or any numeric conversions (e.g. hex↔decimal, size/4), use Python or a terminal command instead of mental math. LLMs are unreliable at arithmetic. -- **Order matters**. Functions and global variables must appear in the same order as in the original binary. The order is determined by the address in `symbols.txt`. **Important**: when `-inline deferred` is used, which it is for most game code, this order is **reversed**. If in doubt, verify. If you move anything around, verify. +- **Order matters**. Functions and global variables must appear in the same order as in the original binary. The order is determined by the address in `symbols.txt`. **Important**: when `-inline deferred` is used, which it is for most game code, this order is **reversed**. The JAudio TUs do not use it, so their order follows the map forward. If in doubt, verify. If you move anything around, verify. - **Include order matters**. It affects BSS layout and static initialization order. - **Template instantiation** is explicit in the binary — each instantiation is a separate symbol. - **The compiler is MWCC, not GCC/Clang**. It has unique optimization behavior, instruction selection, and register allocation. Code that looks equivalent in C++ may compile differently. The only ground truth is `decomp-diff.py` comparison. -- **`#pragma dont_inline on/off`** is sometimes needed to prevent the compiler from inlining a function when matching requires a standalone copy to exist. This is a temporary fakematch which can hopefully be figured out later. +- **`#pragma dont_inline on/off`** is sometimes needed to prevent the compiler from inlining a function when matching requires a standalone copy to exist. This is a temporary fakematch which can hopefully be figured out later. The real cause is almost always the inline pass ladder: a wrapper inline, a call in a conditional part, or a callee over the statement limit (see the tips document). +- **A high match percentage does not mean correct behavior.** objdiff ignores register names and scores a different offset as a small difference, so a logic bug can hide in the last 1-3%. Real gameplay bugs were found in 97-99.8% functions: + * a field load at the wrong offset (`lfs 0x50` where the ROM has `lfs 0x58`), or a wrong array index, + * a wrong bit in a mask (`rlwinm.`/`andi.`), or a wrong constant in a compare, + * a virtual call through the wrong vtable slot (only the `lwz r12, N(r12)` displacement differs), + * a `bl` to the wrong function, + * a virtual call on the wrong receiver, or two values stored into swapped slots, + * a misplaced closing brace (only one branch target differs), + * a wrong stack local passed to a call, hidden because every `r1` offset already differs in a frame-short function, + * `max` written where the ROM has `min`. + + For each `~` line in a diff, check the data flow: which field offset feeds which store, which register is the call receiver, which arm writes the result. + Do not stop at the percentage. +- **A function in `mario.MAP` but not in `symbols.txt` is not missing.** `symbols.txt` lists only what survived in the DOL. Grep the map for the mangled name. ### Matching workflow for agents - **One change at a time**. Make a single source-level change, rebuild with `ninja`, and check the diff immediately. Even small changes can have non-local effects on register allocation and instruction scheduling. - **Prioritize structural differences**. - - First of all, control flow and function call order must be matched. Sometimes, the control flow might not look stuctured and it might seem like a `goto` was used. In almost 100% of the cases, this is actually an inlined call to a different function with early returns inside of it. Introducing a TU-local (`static`) fabricated helper is a good first step, which can later be promoted to a method or one of the UNUSED functions in the TU. Function call ordering is trivial to match, unless inline functions are involved. When experiencing inlining issues, pragmas or additional wrappers can be temporarily employed to force a match. Remember to always mark them as fabricated/fake/needs further work. + - First of all, control flow and function call order must be matched. Sometimes, the control flow might not look stuctured and it might seem like a `goto` was used. In almost 100% of the cases, this is actually an inlined call to a different function with early returns inside of it. Introducing a TU-local (`static inline`) fabricated helper is a good first step, which can later be promoted to a method or one of the UNUSED functions in the TU. Function call ordering is trivial to match, unless inline functions are involved. When experiencing inlining issues, pragmas or additional wrappers can be temporarily employed to force a match. Remember to always mark them as fabricated/fake/needs further work. - Secnd, order of loads/stores to memory must be matched. Any function call in MWCC invalidates all cached loads, most stores to memory do too, and this can be used to figure out where exactly loads and re-loads happen in the original code. - Lastly, in the diff output, `|` (opcode replacement), `<` (deleted), and `>` (inserted) are the real issues. `~` (operand-only) mismatches are usually just register renumbering or stack offset differences that resolve once structural issues are fixed. - **Don't try to process diff tool output with external utilities**. It is designed to be read by agents, and filtering out certain lines via regex will miss crucial context. The diff shows the disassembly of the original binary and current code simultaneously and should be read directly and sequentially in an attempt to understand what the current code does differently from the original. @@ -448,6 +678,20 @@ UNUSED functions must still be reconstructed in the source because: - **Prove every hunk.** Before opening a PR, revert each change on its own, rebuild, and keep only those that move the object toward the target. - **Follow the file's own idioms.** Use a spelling only if an already-matching function in the same file or directory writes it the same way. A form the rest of the file never uses is usually a fakematch. - **Read [docs/AGENT_MATCHING_TIPS.md](docs/AGENT_MATCHING_TIPS.md)** for detailed MWCC codegen patterns that come up repeatedly. +- **When two spellings both match, choose the one that invents nothing.** A plain local beats a new helper, and a helper that the map proves beats one that it does not. +- **Rename in small batches.** For each renamed field, state the evidence. Then run `ninja changes_all` and confirm that the objects did not change. + +### Editing files on Windows + +These traps have emptied or silently skipped edits before: + +- Do not edit source files with a `python -c` one-liner. Write a script file, and pass literal strings through a JSON file. +- `open(p, 'wb').write(open(p, 'rb').read())` truncates the file before it reads it. +- PowerShell 5 removes embedded `"` from the arguments of native programs, so a replacement with quotes silently fails to match. +- Git Bash `sed -i`, and Python text mode with `newline=""`, change CRLF files to LF. `git diff` shows nothing, but `git status` shows the file as modified. +- `Set-Content -Encoding utf8` in PowerShell 5 adds a BOM. + +After each scripted edit, check the result with `git diff --stat`. ### Common review feedback @@ -473,8 +717,12 @@ Before submitting a PR with matching work, make sure to check the following: * For each TU, every function available in the map, including UNUSED ones - Whether functions inside of each TU are defined in the same order as they are listed in the mario.MAP file (or reverse order if the unit is -inline deferred) - If some UNUSED symbols were filled out with guessed code from inline sites -- check that the function size matches the size listed in mario.MAP -- Check that UNUSED symbols are defined inside of the **cpp file** and not the header -- UNUSED symbols are never weak so cannot be inline -- Check that all weak symbols are actually marked as inline in case of free functions or define in-line inside of the class in case of methods. Note that this does not include UNUSED symbols, as they are **never** weak +- Check that UNUSED symbols are defined inside of the **cpp file** and not the header. + An `inline` function that MWCC expands at every site is never emitted, so it leaves no map entry at all. + An UNUSED entry therefore means that the TU emitted an out-of-line copy, which is normally an ordinary non-inline function. + An UNUSED symbol can be weak only when the code that referenced it was dead-stripped too (the map does not record the binding of UNUSED symbols) +- Check that all weak symbols are actually marked as inline in case of free functions or define in-line inside of the class in case of methods +- Check that no fabricated helper is plain `static`: a helper with no UNUSED map entry must be `inline` - Whether the code uses properly uses available inlines of other classes (e.g. checkLiveFlag instead of direct access) - Re-validate GX code using decomp-diff.py to see whether correct constants were used everywhere (e.g. GX_COLOR0 vs GX_COLOR0A0) - Validate const-ness of global variables -- they get emitted to different sections depending on if const is present or not @@ -483,4 +731,7 @@ Before submitting a PR with matching work, make sure to check the following: - Validate that all virtual functions in each class are properly marked as virtual (even if they are overriden) and are sorted in the order they appear inside of the virtual table - After a header change, rebuild everything and confirm no other unit lost a match. - For a unit that is empty on main, the symbol check allows nothing. Every symbol the map lists for it, weak and template copies included, must exist in the object. -- CI also checks the PAL build; don't change or invent `VERSION_GMSP01` code you can't verify against it. +- Run `ninja baseline` and `ninja changes_all` for **every** supported version (see [Build & Diff Workflow](#build--diff-workflow)), and confirm that each DOL still reports `OK`. +- CI also checks the other versions; don't change or invent `VERSION_*` code you can't verify against that version. +- Check that no pad, probe, marker call or `volatile` frame trick is left in the source. +- Commit messages are one line, with no `Co-Authored-By` line. diff --git a/docs/AGENT_MATCHING_TIPS.md b/docs/AGENT_MATCHING_TIPS.md index 8095b6173..becd9e1dd 100644 --- a/docs/AGENT_MATCHING_TIPS.md +++ b/docs/AGENT_MATCHING_TIPS.md @@ -1,99 +1,161 @@ # MWCC Matching Tips for Agents -This document collects practical knowledge about how the Metrowerks CodeWarrior C++ compiler (MWCC GC/1.2.5) generates PowerPC code. These tips help when iterating on source code to achieve a byte-identical match against the original binary. +This document collects facts about how Metrowerks CodeWarrior (MWCC `GC/1.2.5`) makes PowerPC code. +Use these facts when you change source code to get a byte-identical match against the original binary. -Read this document before attempting to match functions. The patterns described here are recurring and will save significant trial-and-error time. Expand the document as needed. Ask for human review whenever the contents of the document disagree with observed reality. +Read this document before you try to match functions. +The patterns here occur again and again, and they save much trial and error. +Add to the document when you find a new pattern. +Ask for human review when the document does not agree with what you see. -## What NOT to focus on +Each fact here was measured on this project with this compiler. +Do not use codegen facts from other decomp projects (TP, TWW, MKDD, AC): each one uses a different MWCC version. +From other projects, take source-level facts only: names, field order, control flow, whether a function or an assert existed. -There are some things that might seem like low hanging fruit at first sight but really are not. They are: +## Terms -- function stack frame size -- prologue and epilogue shape -- wrong registers being used +This document uses these terms with one meaning only: -These things are usually the hardest ones to get right and so should be left for the very end of matching an entire file, when nothing else remains to be done. -Instead of these, first and foremost wrong instructions being used and ordering of these instructions should be resolved. +- **ROM** or **target**: the original object code, extracted from the DOL. +- **site**: one place in the source that calls a function or reads a field. +- **inline expansion**: MWCC copies the body of a callee into the caller at a site. +- **inline pass**: one sweep of MWCC's inliner over the statement list (see [Inlining](#inlining)). +- **temp**: an object that the compiler makes and that has no name in the source. MWCC names temps `@NNN`. +- **slot**: one stack location that MWCC reserves for a local or a temp. +- **frame**: the full stack frame of a function, set by `stwu r1, -FRAME(r1)`. +- **frame deficit**: the frame of our build is smaller than the ROM frame. A **frame surplus** is the opposite. +- **frame-only**: every instruction matches, and only the frame size or the `r1` offsets differ. +- **vreg**: a virtual register, before register allocation. +- **node**: a vreg in the interference graph of the register allocator. +- **coalesced node**: a node that MWCC merged with a physical register, usually an argument register. +- **web**: one live range of a variable. A variable that gets a new value starts a new web. +- **UNUSED**: a symbol that `mario.MAP` lists with a size but that the linker removed. +- **fabricated**: a name or a function that has no evidence in the map, in strings or in a reference. -## Function definition order matters +## Where to start -Functions with non-weak symbols must be defined in code in the exact order they appear in the target binary. -MWCC never reorders symbols, so this is a rule that must never be broken. +### What NOT to focus on first -## Do not use reinterpret casts to access fields +Some differences look easy, but they are the most difficult to fix: -Never use raw byte offsets and reinterpret casts to access fields -- the codegen from that almost always differs from using proper fields. -Instead, introduce the relevant fields and use them, or rather even getters/setters if already available. +- the frame size, +- the prologue and epilogue shape, +- wrong registers. -## MWCC dislikes reordering +Fix these last, after everything else in the file matches. +First fix wrong instructions and the order of instructions. -The compiler never reorders memory stores, loads and function calls relative to one another. -They always should be performed in source code in the exact order they appear in target assembly. -This a good first place to focus on when matching, as it helps shape the rest of the function. +### Priority order -## MWCC can eliminate redundant reads, but not writes +1. Match the control flow and the order of calls. +2. Match the order of loads and stores. +3. Match the instruction selection. +4. Match the registers. +5. Match the frame. -All writes to memory that happen in code will be reproduced in assembly, and vice-versa, when reconstructing code from assembly, you MUST do repeated redundant stores into memory -- that's just the way the original code was written. +Steps 4 and 5 often fix themselves when steps 1 to 3 are correct. +Step 4 can depend on step 5: the extra temps of a correct inline change the order in which MWCC colours registers. +Thus, do not try to fix a register swap before the inline depth is correct. -Redundant reads, on the other hand, can be eliminated by MWCC, but not always, e.g. function inlining might inhibit this. +### Use the compiler sources and the MWCC debugger -## MWCC is very reluctant to optimize anything that uses stores ints to memory +A complete, 100% matching decompilation of `mwcceppc.exe` exists ([`rayanht/mwcc`](https://github.com/rayanht/mwcc); it covers GC 1.0 to 1.3, including 1.2.5). +It is usually at `../mwcc`. If it is not there, ask the human to clone it (see [AGENTS.md](../AGENTS.md#read-the-compiler-instead-of-guessing-the-mwcc-decompilation)). +Read it before you guess about codegen. -If a Vec struct that contains 3 floats is copied via it's compiler-generated copy ctor or assignment operator -- it will be compiled to integer loads/stores. This makes the compiler unable to keep the values used in registers and forces it to spill them to stack, even if the surrounding code clearly allows for them to stay in floating point registers. +| Topic | Files | +|---|---| +| Parser, expressions, inlining, templates | `src/frontend/` (`CExpr.c`, `CInline.c`, `CTemplate*.c`, `CClass.c`, `CDecl.c`, `CParser.c`, `CMangler.c`) | +| IR optimizer: propagation, CSE, struct scalarization | `src/optimizer/Iro*.c` (`IroCSE.c`, `IroPropagate.c`, `IroSubable.c`, `IroVars.c`, `IroLinearForm.c`) | +| Code generation, register colouring, scheduling, stack frame | `src/backend/` (`CodeGen.c`, `Coloring.c`, `InterferenceGraph.c`, `Scheduler.c`, `StackFrameEABI.c`, `Peephole.c`, `StrengthReduction.c`) | -Same logic applies to a Color struct that contains four 8-bit ints: if the ints are initialized one by one, then the compiler will not be able to optimize it to simple bit manipulations in integral registers and will keep the struct on the stack. +The `mwcc-izer` tool runs the real `mwcceppc.exe` and dumps its internal state after each pass. +It runs `GC/1.2.5` natively, so its dumps are exact for SMS. +If a human gave you the tool, use it like this: -## MWCC 1.2.5 stack padding bugs - -Our version of MWCC has a bug where the backend allocates more stack than necessary. - -Most commonly this happens when functions were inlined: inlined function calls often inflate the stack frame size. To correctly match a function with stack frame size issues, UNUSED inlines from the MAP need to be reconstructed based on their size, and sometimes new inlines need to be fabricated based on own's best judgement. - -Another instance of it is using a ternary operator sometimes taking up more stack than using ifs. - -Next, local variables can expand the stack even if they are always stored in a register and never actually spilled to the stack. - -When no obviously correct way to make stack frame size match exists, a trick should be used to correctly match the function's context: a temporary char array of required size to inflate the stack. Such hacks however should be removed or commented out after the function is matching to allow for a possible proper solution in the future. - -## Reading frame-size differences - -The frame is `8 + locals rounded up to 8 + saved GPRs × 4 rounded up to 8 + saved FPRs × 8`. A 4-byte change in locals can therefore move the frame by 8 or by 0. Each inlined call site keeps its own temporaries, sibling branches don't share them, and a loop body counts once. A frame that is too small usually means the original had more inline expansions or deeper inline chains at the same instructions. It rarely means a missing local. - -## Ifs - -Ifs are always compiled to very simple code: -- compare (`cmpwi`/`cmplwi`/etc, or arithmetic instruction with a dot) -- conditional branch (`beq`/`bne`/`ble`/etc) -- the true block -- unconditional branch to end of false block (`b`) -- the false block - -The compiler **NEVER** swaps the order of the true block and false block. -It is also very reluctant in changing the control flow, so C++ control flow usually corresponds to assembly one to one. - -Ternary operator is compiled similarly, but it is the one exception to control flow being the same. In the following case MWCC might initialize the variable's register with the "otherwise" value (zero) instead of doing so in the false branch, which eliminates the false branch entirely. ``` -int b = thing == nullptr ? thing->field : 0; +python mwcc_debugger.py -C -a ' -sym on -c src/...' +python mwcc_izer.py stack # owner of each frame slot +python inline_trace.py -u --root -C # inline verdict per callee per pass ``` -## Sequential integer comparisons in a disjunction +Get the command line from `ninja -t commands `, and remove the `sjiswrap` and `transform_dep.py` wrappers. +To find a mangled name, give a name that does not exist: the tool prints each function that it skips. -Whe MWCC sees code like `if (a == 8 || a == 9 || a == 10)` it can optimize it to be `if (a - 8 <= 2)` sometimes. When the latter pattern is encountered with enums -- it should be reversed into multiple disjuncted equality comparisons. +The most useful dumps: -## Assigning a `&&`/`||` chain to a bool synthesizes intermediate bools +| Dump | Use | +|---|---| +| `variables.txt` | Every local and temp, with its register or its `r1` range. This is the frame layout. | +| `frontend-00-ast-initial-code.txt` | The AST after inlining. Shows which inline expansion made each temp. | +| `backend-00-initial-code.txt` | Read this first. If a difference is already here, the front end made it. Change the AST shape. | +| `regalloc-gpr-pass-1-all.txt` | The full interference graph: each node, its register, `fCoalesced`, and its neighbours. | +| `regalloc-gpr-pass-1-assigned.txt` | Nodes in colouring order, highest priority first. | -When a multi-term short-circuit expression is **assigned to a `bool` variable**, MWCC materializes its own intermediate boolean temporaries for the chain — it initializes a flag register to `0`, sets it to `1` when a sub-condition passes, and merges flags for each grouping level. -This produces a distinctive shape: several `li rN, 0` up front, then -`li rN, 1` at the deep success points, then `clrlwi.`/`beq` merges. +`inline_trace.py` shows `[N stmt]`, which is the statement count that the inline pass ladder uses. + +The sweep scripts change the real source file between builds, and objdiff watches that file. +Restore the file at the end of each script. + +## Control flow + +### Ifs + +An `if` always compiles to simple code: + +- a compare (`cmpwi`, `cmplwi`, or an arithmetic instruction with a dot), +- a conditional branch (`beq`, `bne`, `ble`, ...), +- the true block, +- an unconditional branch `b` to the end of the false block, +- the false block. + +MWCC **never** swaps the true block and the false block. +It also changes the control flow very little, so C++ control flow usually agrees with the assembly one to one. +Use this to read the polarity of a condition: `cmplwi r31, 0; beq L_one` with `li r0, 1` last means the source tested `p != nullptr`, and the null case is the second arm. + +To find where an `if`/`else` ends, find the unconditional `b .L_X` at the end of the true block. +The code at `.L_X` is the first statement after the `if`. +A misplaced closing brace changes only one branch target, so objdiff shows almost no change. + +### Ternaries + +A ternary compiles like an `if`, with one exception. +MWCC can load the "else" value into the result register before the compare, and then the false block disappears: ```cpp -// Target has flag-register merges (r0/r4/r3 = 0, then =1 at success, then merged). -// This form reproduces them: +int b = thing != nullptr ? thing->field : 0; +``` + +Use this signature to tell a ternary from an early return: + +- A branch target whose only instruction is `fmr` or `mr` into the return register is a ternary. +- A branch target that loads a constant is an early `return`. + +```cpp +if (d < 0.0f) return 0.0f; // the constant is loaded inside the taken branch +f32 x = d < 0.0f ? 0.0f : d; // the constant is loaded before the compare +``` + +Write a chained clamp as two ternaries with a named intermediate, not as one nested ternary. +The compare keeps the operand order of the source: `a > b` gives `fcmpo cr0, a, b`. + +### Sequential integer compares + +MWCC can change `if (a == 8 || a == 9 || a == 10)` into `if (a - 8 <= 2)`. +When you see the second shape with an enum, write it back as separate `==` compares. + +### `&&`/`||` chains assigned to a bool + +When you assign a multi-term `&&`/`||` expression to a `bool`, MWCC makes intermediate flag temps. +The shape is several `li rN, 0` first, `li rN, 1` at the success points, then `clrlwi.`/`beq` merges. +These come from the `CInline_LoadToTemp` pair in `CInline_SerializeELOR`. + +```cpp +// Target has flag-register merges. This form makes them: bool same = (-e <= dx && dx <= e) && (-e <= dy && dy <= e) && (-e <= dz && dz <= e); if (!same) { ... } -// Nested ifs do NOT — they compile to early-exit branches straight to the body, -// with no intermediate flag bools: +// Nested ifs do NOT make them. They branch to the body directly: bool same = false; if (-e <= dx && dx <= e) if (-e <= dy && dy <= e) @@ -101,17 +163,25 @@ if (-e <= dx && dx <= e) same = true; ``` -When the target shows those `li 0`/`li 1`/merge flags for a compound predicate, spell it as a single `bool x = cond && cond && ...;` assignment (grouped to match the flag nesting), not as nested `if`s. +Group the terms to agree with the nesting of the flags. +Case: `MtxUtil.cpp` `TRope::constraintTail` went from 89.3% (nested ifs) to 99.8%. -Concrete case (`MtxUtil.cpp` `TRope::constraintTail`): nested-if near-zero test 89.3%; -`bool same = (-e<=dx&&dx<=e) && ...;` with `<=` and inline `-JGeometry::TUtil::epsilon()` -→ 99.8%. +A **ternary chain** assigned to a bool is different. +It writes the destination register directly in each arm, so the whole chain uses **one** register. +The ROM shape is several `li rN, 0` and one `li rN, 1` in the same register, then one `clrlwi. rN, rN, 24`. +An `&&`/`||` chain uses two or more registers, a `cror`/`mfcr`/`extrwi` merge, and `mr` moves. -## Switches +A comparison written as a ternary arm gives `mfcr`/`extrwi`. +If the ROM branches there, the arm is a ternary itself: `x < y ? false : true`, not `x >= y`. + +An `&&` inside a ternary condition is computed as 0 or 1, not as a short circuit. + +### Switches + +MWCC compiles a switch as a jump table or as branches. +A jump table uses `mtctr` and `bctr`. +A switch compiled as branches has more than one conditional branch after one compare: -MWCC can compile switches in one of two ways: jump table or branching. -Jump tables are easily identifiable via `mtctr` and `bctr` instructions being used. -Switches that became branches usually have control flow that doesn't look like an if: multiple conditional branch instructions follow a single comparison instruction. E.g. ``` cmpwi r0, 0x1 beq ... @@ -122,222 +192,180 @@ b ... ... the code block inside the switch ... ``` -### Comparison constants can be default-range boundaries +MWCC **never** reorders the cases. +The order of the `case` labels must agree with the order in the jump table or the branch chain. -MWCC's `build_case_ranges` sorts case values and inserts default ranges between them. -`emit_case_range_binary_search` can compare against the beginning of a default range, so a comparison constant need not be an explicit `case` value. -Read these routines in the MWCC decompilation when the switch dispatch differs despite apparently correct case values. +A compare constant is not always a `case` value. +`build_case_ranges` sorts the case values and adds default ranges between them. +`emit_case_range_binary_search` can compare against the start of a default range. +Case: in `conductor.cpp` `TConductor::isBossDefeated`, `default` grouped with `case 1`, then `case 3`, gives compares against `2` and `4`. +`2` is the default gap between the explicit cases, and `4` starts the default range above `3`. -Concrete case (`conductor.cpp`, `TConductor::isBossDefeated`): `default` grouped with `case 1`, followed by `case 3`, reproduces the target comparisons against `2` and `4`. -Here `2` is the single-value default gap between the explicit cases, and `4` starts the default range above `3`. -Using `case 2` instead gives a different decision tree. -The corrected switch matches; the function remains frame-only nonmatching. +### Control flow that looks like a goto -### Case ordering is strict +Inline expansion can make control flow that agrees with no `if`, `switch` or loop. +A jump that looks like a `goto` is almost always a `return` inside an inlined function. +The jump target is the end of the inline expansion. +Write a helper with early returns. +A fabricated helper must be `inline` (or `static inline`), never plain `static`. +A plain `static` function stays in the object after MWCC expands all its sites, and then the map must list it as UNUSED. +If the map has no UNUSED entry for it, the helper was not a `static` function. -MWCC **never reorders cases in a switch statement** during compilation. The order of `case` labels in the source code must match exactly the order they appear in the assembled jump table or branch chain. If a function exhibits a non-matching switch and you've verified the case logic is correct, inspect the target assembly to determine the actual case sequence and reorder the source cases accordingly. Even if semantically equivalent, the compiled output will differ until case order matches. +### Release-only debug code -## Nonsensical control flow - -As MWCC inlines functions, sometimes nonsensical control flow will be encountered in the assembly, one that doesn't correspond to any structured control flow constructions like switches, ifs or loops. Such cases are usually explained by **function inlining** rather than gotos. The place where a goto was supposedly used would actually correspond to a return statement, and the place where it points to would be the boundary of the inlined call. - -## Keep track of known relevant inlines - -Reconstructing correct inline calls is crucial in matching code correctly. When a similar block of code reoccurs -- always consider the possibility that it's an inline, but never disregard the possibility that the original authors simply copy-pasted it. When starting on a new function, explore the inlines already available in the different classes that it uses, as well as in the current translation unit. - -UNUSED functions from the MAP must often be reconstructed even when they do not exist as standalone code in the final binary: their inlined bodies still determine caller codegen (register allocation, load offsets, branch layout). Treat their MAP signature and size as constraints, recover a plausible body from repeated callsite patterns, and validate by diffing the caller(s) after each inline-shape change rather than expecting a direct symbol-level match for the UNUSED function itself. - -### A getter that repeats before every operation is a wrapper inline - -m2c writes an inlined wrapper as one pointer local that is assigned again and again: +A release build removes debug warnings and asserts, but the **form** of the removed code changes the registers around it. ```cpp -pBuf = DSPInterface::getDSPHandle(channel->unk0); -pBuf->setPauseFlag(1); -pBuf = DSPInterface::getDSPHandle(channel->unk0); -pBuf->flushChannel(); +if (sVir2PhyTable[vir_id] != 0xFFFF) { } // the compare stays as dead nodes +if (sVir2PhyTable[vir_id] != 0xFFFF) (void)0; // the dead compare goes away early ``` -When the same lookup comes back before every single operation, the original source almost always called a wrapper that hides the lookup: +With the empty compound statement, MWCC computes the compare as a value. +The dead instructions are removed only after register colouring, so they take registers. +With `(void)0`, the condition is still evaluated (the load stays), but the compare chain never reaches the back end. +Case: `JASystem::BankMgr::setVir2PhyTable` went from 77.1% to 100%. -```cpp -DSPInterface::setPauseFlag(channel->unk0, 1); -DSPInterface::flushChannel(channel->unk0); -``` +Use the existing macros: `JUT_ASSERT(...)` and `JUT_REPORT_MSG(...)` expand to `(void)0` in release builds. +Do not write a raw `#ifndef NDEBUG` block in braces, and do not leave `if (cond) { }`. -Both forms give the same instructions, but the wrapper form reads correctly and it colours the registers correctly in long functions. -`JAInter::StreamLib::callBack` went from 98.7% to 99.8% on this rewrite alone. -Keep the pointer local only where the code **reads a field** through it. +`JGADGET_ASSERT` and `JUT_ASSERT` are not no-ops for matching. +The empty statement still reaches the front end and changes temp numbering, the frame and register colouring. +Removing the body of `JGADGET_ASSERT` dropped three JGadget functions to below 67%. +An assert inside an inlined accessor can add a frame slot by itself. +Port every assert that a reference supports, as `JUT_ASSERT();`. Asserts in this project have no line numbers. -## Check out-of-line copies before changing an inline +A release-stripped `JUT_ASSERT(...)` is a `(void)0;` statement, and `(void)0;` is an `ST_EXPRESSION`. +It counts for the 15-statement gate and for the budgets of the pass ladder. +**One** assert can stop the inlining of a callee that is at the limit (see the table in [Is the callee inlinable at all](#is-the-callee-inlinable-at-all)). -When MWCC declines to inline something, it emits a weak or local copy into the caller's unit, and `mario.MAP` lists it. That copy is the original body. Diff it before changing the inline. If the ROM calls an inline out of line at some sites and inlines it at others, the difference is at those call sites, not in its body. +### A dead `subfc`/`subfze` pair -## Narrowing casts are a hint +MWCC computes `unsigned >= 0` with carry arithmetic, not with a branch: `li 0`, `li -1`, `subfc`, `subfze`. +When nothing uses the result, the users go away but the arithmetic stays. +In a loop, only the two `li` stay, hoisted into the preheader. -A `clrlwi`/`extsh` before a use rarely means the original had a cast. Look first for a parameter, return, local or member type that narrows the value. +- `(cond) ? (void)0 : (void)0;` also removes the second half of an `&&`. +- `(void)(cond);` keeps it, so the upper-bound load and compare stay. -## Signedness and width show in the compare +`x == -1` on an unsigned value gives `addis r0, rX, 1` / `cmplwi r0, 0xffff`. That is a real compare. -`cmpwi` vs `cmplwi` and `extsb`/`extsh` vs `clrlwi` give the sign and width of a value. Fix the declared type rather than adding casts. +## Inlining -## Reference Locals Affect Register Allocation +### Is the callee inlinable at all -Introducing a reference local before accessing struct members can change how the compiler allocates registers: +For a body in a `.cpp` file, `CInline_GenFunc` marks a function as inlinable only when two checks pass: -```cpp -// Direct access — compiler may reload base pointer each time: -unk0[i].mPos.set(...); -unk0[i].mVel.set(0.0f, 0.0f, 0.0f); +- **`CInline_IsSmallFunction`**: at most **15 statements**, and at most 1024 bytes of locals. +- **`CInline_CanInline`**: the result is one of three values. + - `CI_CanInline0` (never): varargs, a class return value whose class has a destructor, or a by-value class parameter whose class has a destructor. + - `CI_CanInline6`: the body is only expression statements and one final `return`. + - `CI_CanInline3`: the body has control flow. -// Reference local — compiler keeps base in a register: -Node& node = unk0[i]; -node.mPos.set(...); -node.mVel.set(0.0f, 0.0f, 0.0f); -``` +The mark is on the function, so it is the same at each site. +**A difference between two sites is never caused by the callee.** -Check whether the target reloads the base address or reuses a register. If it reuses, a reference local is likely in the original code. +The 15-statement gate counts a flat statement list: -## Inlined Functions Only Load `this` Once +| Source | Statements | +|---|---| +| Declaration with an initializer (also `T& r = x;` and `T* p = &x;`) | 1 | +| Declaration without an initializer | 0 | +| `if (c) x;` | 2 (`if` plus the body statement; the label is free) | +| `(void)0;`, and so a release-stripped `JUT_ASSERT(...)` | 1 | +| an empty statement `;` or an empty block `{}` | 0 | +| `break`, `continue` | 1 | +| `while` or `for` | 4, the same for both | +| `return`, also the implicit `return` at the end of a `void` function | 1 | +| an `&&` chain in one `if` | still 1 | -Whenever a simple implementation loads a pointer into a register multiple times but the original did it once — the original might have used a function that got inlined. +The gate counts every statement except `ST_NOP` and `ST_LABEL` (`check_statement_count_and_locals_size` in `CInline.c` of the decompilation). +Measured with `GC/1.2.5` on a `void` callee: 14 stores inline, 15 stores do not, and 14 stores plus one `(void)0;` or one `JUT_ASSERT(p);` do not. -```cpp -// `field` pointer could get loaded twice because -// the compiler assumes function calls can modify any memory -field->nonInlinedFuncCall1(); -field->nonInlinedFuncCall2(); +Read the real count from `frontend-00-ast-initial-code.txt`. +If the target inlines a function that our build calls, the original body was 15 statements or fewer. +Look for a helper that replaces a group of statements with one call, or fold `if (c) continue;` guards into one `&&` condition. +If the target calls a function that our build inlines, the original body was over the gate. +A named local for a value that the code already computes adds exactly one statement. -// `field` pointer loaded as `this` once -field->inlineFuncThatCallsBoth(); +Case: on `JASystem::Dvd::loadToDramDvdT`, a five-line block was really a call to the UNUSED `Dvd::addTask`. +That took the function from 16 to 12 statements, and its caller from 32% to 100%. -... +### An in-class body has no gate -class FieldClass { -... - void inlineFuncThatCallsBoth() { - nonInlinedFuncCall1(); - nonInlinedFuncCall2(); - } -... -}; -``` +A body inside the class body sets `Q_INLINE`. +For such a function, MWCC does not run `CInline_IsSmallFunction` and does not read `#pragma dont_inline`. +The same constructor inlined with 14, 16 and 26 statements. -This is sometimes indistinguishable from a local reference being used. +A member of a **class template** defined outside the class body is never marked inlinable: it is always a `bl`. +Move the body into the class to inline it at every site. +This changes every site, so do it only when every caller in the ROM inlines it. -## Intermediate Variables Heavily Influence Register Allocation +### The inline pass ladder -Intermediate variables are not inherently bad. They are a strong codegen lever in MWCC and can change register pressure, load/store timing, and expression scheduling. +`CInline_Expand` sweeps the statement list once per pass. +Each pass permits a smaller callee: -Start with the simplest hypothesis, then iterate: -1. First try a fully inlined arithmetic form (no intermediate variables). -2. If that does not match, try decomposed forms with temporaries. -3. If still nonmatching, vary grouping/order (including compound assignments). +| Pass | Largest callee (`[N stmt]` in `inline_trace.py`) | +|---|---| +| 0: calls that the source writes | no limit | +| 1: calls that a pass-0 expansion uncovered | 10 | +| 2 | 7 | +| 3 | 3 | -Examples (all semantically equivalent, all can compile differently): +**There is no pass 4.** +MWCC never scans a body that a pass-3 expansion inserted, so every call in it stays a `bl`. +This is true also for a one-statement accessor that MWCC inlines everywhere else, and MWCC then emits a weak copy of it in that TU. +The signature: a `bl` to a trivial inline at one place, and the same inline expanded a few instructions before. -```cpp -// Inline form -f32 c0 = (z1 - z0) * (x2 - x1) - (x1 - x0) * (z2 - z1); +How the depth grows: -// Decomposed form A -f32 dx = x1 - x0; -f32 dz = z1 - z0; -f32 c0 = dz * (x2 - x1) - dx * (z2 - z1); +- An expansion returns its last statement, so statements that a pass makes are first seen in the next pass. +- A `CI_CanInline6` body pasted as an expression is not walked again in the same pass. +- Calls nested in one statement's arguments are all walked in the same pass, so `a()->b()->c()` costs one pass, not three. +- **A call in the argument of an expanded call moves down one pass.** In `stack.push(fetchS32())`, `push` is pass 0, the implicit `TSpcSlice(int)` conversion is pass 1, and `fetchS32` is pass 2. +- A call nested inside an already expanded body costs no extra pass. -// Decomposed form B -f32 c0 = (z1 - z0) * (x2 - x1); -c0 -= (x1 - x0) * (z2 - z1); -``` +To push one callee deeper than its sibling, move the call into the other callee's body. +Do not nest it as an argument. -Practical rule: do not treat temporaries as "wrong" or inlining as "always right". Treat decomposition style as an iterative search dimension and test multiple shapes until one matches. When trying different approaches, always try things that look reasonable to have been written by a human first, and only if nothing sensible works start trying crazy and weird approaches. +Case: `std::sqrtf` is 9 statements. +Every ROM `frsqrte` site is at pass 0 or 1, and every `bl sqrtf__3stdFf` site is at pass 2 or deeper. +To keep one call out of line, add one inline level above it. +Never add statements to `sqrtf`. -## Avoid Reference Aliases as a Workaround +### Why a pass-0 call can stay a `bl` -When trying to match complex struct access patterns, DO NOT use reference aliases as a general strategy to "improve" codegen. While they can sometimes help with register reuse (see "Reference Locals Affect Register Allocation" above), they frequently harm matching by changing pointer offset calculations and register assignments in ways that deviate from the original. +A call statement at pass 0 is always expanded, except in these cases: -Only introduce reference aliases when: -1. The original code clearly had them (evidenced by a single load of a base pointer that's reused) -2. Direct member access results in repeated redundant loads that don't appear in the target -3. You are confident from diff analysis that a reference was intended +1. **The call is in a conditional sub-expression**: the right operand of `&&`, `||` or `,`, an arm of `?:`, or a null-check condition. + There, a `CI_CanInline3` callee stays a call. + A `CI_CanInline6` callee is pasted in anyway. + An `if` condition is not a conditional part. + In `new T(...)`, the constructor call is on the unconditional side, so even a `CI_CanInline3` constructor expands. +2. **The statement is not an expression statement** (the call is the full expression of a `return`, `if`, `switch`), **and the callee has a `return` before its last statement**. +3. The site came out of another expansion (pass 1 or more), and the callee is over that pass's budget. +4. The callee is `CI_CanInline0`, its body is not compiled yet, it is not inlinable, or it is a virtual call through the vtable. -Otherwise, use direct member access or direct getter calls inline in expressions and let the common subexpression elimination & redundant load optimizations do the rest. +Reason 1 is the clean way to keep one site out of line without a change to the callee. +Case: `JALListD::searchD` is inlined in `searchGroup` but called in `startSoundSetDyna`. +Writing the second site as the right operand of `||` gives both, and MWCC emits the weak copy that the map lists. -## Reconstructing initializer-heavy functions (init/ctors) from raw asm +Case: `TFireWanwan::behaveToWater` calls `isWandering()`. +Inside it, the first `theNerve()` is unconditional and expands, and the same call behind `||` stays a `bl`. -When a function body is mostly field assignments (typical for `init()` / constructors), the fastest path is to decode the raw `.s` file directly rather than relying on `m2c` or guessing. +These things do **not** change an inline decision: -Workflow: +- the shape of the arguments, +- `Class::f(...)` against `this->f(...)`, +- the position of an in-class body in the file (`-inline deferred` generates every function at the end of the TU). -1. Find the function in `build/GMSJ01/asm/.s` by mangled name (`grep -n` for `.fn `). -2. Walk the prologue forward and map every `stb`/`sth`/`stw`/`stfs` to a class field using the `/* 0xNN */` offsets in the header. Each store = one source statement. -3. Float and integer constants live at the bottom of the same `.s` file as `.obj "@NNNN"` blocks with `.float`/`.4byte` directives. The assembly references them as `lfs f0, "@NNNN"@sda21(r0)`. -4. String literals (joint names, file paths) are in `.rodata` / `.sdata2` and referenced as `addi r4, r30, 0xNNN` where `r30` is the loaded address of `@1490` (the TU's local rodata base). -5. Cross-reference against the existing partial source — anything in the asm but not in the source is missing. +`#pragma dont_inline` is read once, at the end of the function. +It cannot turn off inlining for one statement, and it also stops the function that it wraps from being inlinable. +Treat it as a placeholder that hides a real cause, never as a fix. +Case: in `JAISound.cpp`, both `#pragma dont_inline` blocks hid the `U7` wrapper family (`setSeInterFxmixU7` and others). +The wrapper expands at pass 0, and the real setter (15 statements) is then a pass-1 call that the ladder refuses. -Compare offsets one-for-one. If the asm writes to offsets `0xFC, 0x100, 0x104` and the source only zeroes `0x108..0x110`, you're missing a `unkFC.zero();` before `unk108.zero();`. - -## Recognising common MWCC store idioms - -**`u32 → f32` cast** (typical for `(f32)someByte` from a packed struct like `GXColor`): -``` -lbz r0, OFF(rBase) ; load unsigned byte -lis r4, 0x4330 ; high half of magic constant -lfd f1, "@magic"@sda21(r0) ; magic = 4503599627370496.0 (0x4330000000000000) -stw r0, X(r1) ; store low half on stack -stw r4, X-4(r1) ; store high half on stack -lfd f0, X-4(r1) ; load as double -fsubs f0, f0, f1 ; subtract magic → real float value -stfs f0, FIELD(rThis) -``` -Source: `unk84.x = (f32)bodyColor[0].r;` — straight cast of a `u8` field to `f32`. - -**`(u8)` cast on an `int`-returning call before storing to a `u16` field**: -``` -bl JUTNameTab::getIndex(...) -clrlwi r0, r3, 24 ; mask to low 8 bits -sth r0, FIELD(rThis) ; store as halfword -``` -Source: `mField = (u8)getIndex(...);` even though `mField` is declared `u16`. Without the `(u8)` cast, MWCC emits `sth r3, FIELD(rThis)` directly with no `clrlwi`. This pattern appears repeatedly for joint-index assignments. - -## Constant Hoisting and Loop Codegen - -The compiler hoists constant loads (`lfs`, `lfd` from SDA/SDA2) before loops. The exact set of constants hoisted depends on: -- Which expressions appear in the loop body -- Which inline functions are called (they may reference additional constants) -- The order of operations within the loop - -If the target hoists a constant (e.g., `lfd f28, @5181@sda21`) before a loop but our build does not, it means the compiler sees a different code structure. This is usually a symptom of a deeper structural mismatch in the loop body or inlined functions, not fixable by just moving the load. - -## Function-local statics with non-trivial initializers: `init$NNN` is the guard - -When a function-local `static` has a non-trivial initializer that has to run at first call — typically a constructor call like `static JGeometry::TVec3 pos(1815.0f, 1500.0f, 1550.0f);` — MWCC emits **two symbols in `.sbss`**: - -- The variable itself (e.g. `pos$NNN`), sized for the type (a Vec is 12 bytes). -- A 1-byte construction guard named `init$NNN` (note the literal name `init`, regardless of what your source variable is called). - -The function-entry codegen is: - -```asm -lbz r0, init$NNN@sda21 ; load the guard byte -extsb. r0, r0 ; sign-extend with dot (sets CR) -bne .L_skip_init ; already constructed → skip -... initializer code, writes to pos$NNN ... -li r0, 1 -stb r0, init$NNN@sda21 ; mark constructed -.L_skip_init: -``` - -So when the target shows an `init$NNN` byte that is loaded with `lbz` + `extsb.` and tested, **don't** model it as a plain `static bool foo;` in your source — model it as the *guard* for some other static local with a real constructor. Pick the source statement that produces the matching initializer body (commonly a `JGeometry::TVec3(x, y, z)` if the init code writes three floats). - -Aggregate-initialized PODs (`static Vec pos = { 1.0f, 2.0f, 3.0f };`) and zero-initialized statics (`static Vec pos;`) do NOT produce a guard — they sit in `.data` / `.bss` with no first-call check. The `init$NNN` pattern only appears when MWCC needs to run constructor code at first entry. - -## A zero-argument call of a constructor with default arguments costs one inline pass - -MWCC expands inline calls in passes, and each pass permits a smaller callee -(pass 0: no limit, pass 1: 10 statements, pass 2: 7, pass 3: 3, pass 4: none). -A constructor written with default arguments adds one pass to that count when -it is called with **no** arguments: +### A zero-argument call of a constructor with default arguments costs one pass ```cpp class TGameSequence { @@ -352,218 +380,161 @@ TGameSequence local; // synthesized __ct__13TGameSequenceFv, 1 statemen TGameSequence local(a, b); // the real ctor directly, no wrapper ``` -The same rule applies to a member of such a type that the enclosing -constructor default-initialises, for example `TFlagT unkC;` in -`JDrama::TViewObj`. The wrapper has no symbol of its own; it is always -expanded. +The same rule applies to a member of such a type that the enclosing constructor default-initializes, for example `TFlagT unkC;` in `JDrama::TViewObj`. +The wrapper has no symbol, and MWCC always expands it. -How to recognise it: the calls inside a constructor body stay `bl` although -the ladder says they fit, while the same callees are expanded at that depth -elsewhere. `TApplication::TApplication()` keeps `bl TFlagT::TFlagT(const -TFlagT&)` and `bl TFlagT::set(u16)` for each `TGameSequence` member; with a -plain `TGameSequence() { set(0, 0, 0); }` both are expanded and the function -is at 36%. The default-argument form gives 100%. `inline_trace.py` shows the -wrapper as `__ct__XFv [1 stmt]` one pass above the real constructor. +The signature: the calls inside a constructor body stay `bl` although the ladder says that they fit, while the same callees expand at that depth elsewhere. +Case: `TApplication::TApplication()` keeps `bl TFlagT::TFlagT(const TFlagT&)` and `bl TFlagT::set(u16)` for each `TGameSequence` member. +With a plain `TGameSequence() { set(0, 0, 0); }`, the function is at 36%. The default-argument form gives 100%. +`THitActor::THitActor` keeps `bl __ct__...TFlagTFUs` for the same reason. -## A member function called on a bare global substitutes `this`; called through an accessor it binds it +### Use the default argument, do not write it out -When an inline member function is called on a global object written by name, -`gpApplication.setNextArea(x)`, MWCC substitutes `this` with the constant -address, folds the member offsets, and then CSEs `gpApplication + 0x12` into a -callee-saved register: +When a call passes a value that is the callee's default argument, use the default. +Case: `BathWaterManager.cpp`. `JDrama::TViewObj("")` inlined the nested `TNameRef`/`TFlagT` constructors one level too deep. +`JDrama::TViewObj()` kept them as `bl`, like the target, and took the constructor from 90.6% to 100%. -``` -addi r30, r5, 0x12 ; &gpApplication.mNextArea -stb r0, 0x12(r5) -... -addi r3, r30, 2 ; &mNextArea.unk2 -``` +### Keep track of the inlines that exist -When the receiver is an expression that MWCC will not repeat (an inline -accessor call, a local pointer, or a local reference), `this` is bound to a -temporary that holds `&gpApplication` itself, and every offset folds from it: +Correct inline calls are the most important part of a match. +When a block of code occurs again, think that it can be an inline. +But the original authors also copied code, so do not assume. +Before you start on a function, look at the inlines of the classes that it uses and of the current TU. -``` -addi r31, r4, gpApplication@l -stb r0, 0x12(r31) -stb r0, 0x13(r31) -addi r3, r31, 0x14 -``` +UNUSED functions from the map often must exist in the source, although they have no standalone code in the binary. +Their inlined bodies set the registers, the load offsets and the branch layout of the callers. +Use the map's signature and size as constraints, take the body from the call sites, and check the callers after each change. -The second shape, with the global's own address kept across the call, is the -sign that the original went through an accessor such as `SMSGetMSound()` or -`SMSGetMarDirector()`. Nine functions of `System/` show it for -`gpApplication`, and every other `gpApplication` site in the tree compiles to -the same bytes through `SMSGetApplication()`, so the accessor is used -everywhere. Expect the same of the other `SMSGet*` accessors: the bare global -and the accessor differ only where the result feeds another inline call. +A debug build of another game shows only the inlines that something calls. +An inline that no site uses leaves no trace. +The SMS map also lists only functions that got an out-of-line copy somewhere. +So the absence of an inline from TP, TWW or our map is not proof that it did not exist. +A one-line forwarder on an outer class (`a->getBlock()->set(x)` written as `a->set(x)`) is fair to try, if you measure it. -The same test tells a getter from a raw field read. A getter such as -`u8 getStage() const { return unk0; }` reads through `this`, so its return -value is force-loaded into a compiler temporary. Two visible effects: the -temporary keeps a 4-byte slot (a frame 4 bytes short per call is the -symptom), and when two such calls are the arguments of one `bl`, MWCC -evaluates them in the reverse order. `TMarDirector::currentStateFinalize` -loads `mCurrArea.unk1` before `unk0` for `endStageEntranceDemo`, which only -the getters reproduce. An accessor that returns a reference leaves no trace -at all, so it can neither be proved nor disproved this way. +Two changes that each measure as zero can be worth an inline level together. +Case: `J3DModelData::entryTexMtxAnimator`. `J3DMaterial::setTexMtx()` alone changed nothing, a named local for the `new` alone was worse, and both together matched. -## Default arguments vs. spelling them out changes inlining +### A getter that repeats before every operation is a wrapper inline -When a base/member constructor is invoked with a value that happens to be that -constructor's own default argument, prefer the default-arg call form over -repeating the literal. It is not just cosmetic: it can change MWCC's inline -depth decisions. - -Concrete case (`BathWaterManager.cpp`): `JDrama::TViewObj`'s ctor defaults its -name to `""`. Writing a member's base as `JDrama::TViewObj("")` -inlined the nested `TNameRef`/`TFlagT` ctors one level too deep; writing it as -`JDrama::TViewObj()` (letting the default supply the identical string) kept those -nested ctors as real `bl` calls, exactly like the target. This took the -enclosing constructor from 90.6% to 100%. If a nested ctor inlines when the -target keeps it as a call (or vice-versa), check whether the original used a -default argument. - -## Unsized arrays go to `.data`, sized ones may go to `.sdata` - -An array declared with an explicit size that is small enough (≤ the `-sdata` -threshold, default 8 bytes) can be placed in `.sdata` and addressed GP-relative -(`@sda21`, giving an indexed `lwzx` load). An array declared **unsized** — e.g. -`static const char* fileNames[]` with the size deduced from the initializer — -is always placed in `.data` instead, and addressed absolutely (`lis/addi @ha/@l` -+ `add`/`lwz`). - -So if a small global array should live in `.data` (target uses `lis/addi`) but -yours lands in `.sdata` (`@sda21`), drop the explicit array bound and let the -initializer size it. Concrete case (`BathWaterManager.cpp`): `fileNames[2]` → -`.sdata` (nonmatching); `fileNames[]` → `.data` (match). - -## Local Symbol Mangling: `@unnamed@` vs `static` - -MWCC mangles symbols inside anonymous namespaces with an `@unnamed@` prefix. - -- If a local symbol's mangled name includes `@unnamed@`, model it as being in an anonymous namespace. -- If the symbol is local but does not include `@unnamed@`, prefer a plain `static` function/variable instead. - -## Symbol Order with `-inline deferred` - -When a TU is compiled with `-inline deferred` (see TU-specific flags in `configure.py`), define symbols in reverse order relative to the map/symbol listing for that TU. - -- In practice, function-definition order in the `.cpp` should be reversed for those TUs. -- If order-sensitive matching drifts for an `-inline deferred` TU, verify definition order before attempting smaller codegen tweaks. - -## TVec3 / Vector Codegen Patterns - -`JGeometry::TVec3` is a 12-byte struct with `x`, `y`, `z` float members. How you read/write it drastically affects code generation. - -### Use the inlines - -`TVec3` and related `JGeometry` types have lots of inlines, and most of the time the original code would have used those. -When naive decompilation of the math doesn't work out, try using the inlines. - -### Multiplication by zero/one - -MWCC can optimize out multiplication by floating point one or zero written out explicitly in code, but fails to do so when the multiplication comes from an inlined function. -This routinely occurs when an "up" vector is created and then cross-multiplied with another vector -- codegen contains trivial multiplications by zero or one. -When decompiled naively as `1.0f * unk170.z - 0.0f * unk170.y`, MWCC will optimize the multiplications out, but if the "up" vector is properly created and the `cross` inline is used, it will fail to optimize them out, achieving the desired codegen. - -### Construction: component-by-component vs constructor +m2c shows an inlined wrapper as one pointer local that gets a new value again and again: ```cpp -// Constructor form — compiler batches all loads, then all stores: -// lfs f0, ...; lfs f1, ...; lfs f2, ... -// stfs f0, 0(rN); stfs f1, 4(rN); stfs f2, 8(rN) -JGeometry::TVec3 pos(x, y, z); - -// Component-by-component — compiler interleaves load/store pairs: -// lfs f0, ...; stfs f0, 0(rN) -// lfs f0, ...; stfs f0, 4(rN) -// lfs f0, ...; stfs f0, 8(rN) -JGeometry::TVec3 pos; -pos.x = x; -pos.y = y; -pos.z = z; +pBuf = DSPInterface::getDSPHandle(channel->unk0); +pBuf->setPauseFlag(1); +pBuf = DSPInterface::getDSPHandle(channel->unk0); +pBuf->flushChannel(); ``` -Check the target assembly to see which pattern (batched vs interleaved) is used, and write the source accordingly. - -### `Vec3::zero()` vs `Vec3::set(0,0,0)` - -`.zero()` emits stores in **reverse component order** (`z`, `y`, `x` → offsets `+8, +4, +0`). `.set(0,0,0)` typically emits forward order. Match the asm order to pick the right call. - -### Assignment: `operator=` vs `.set()` +When the same lookup comes back before each operation, the original source almost always called a wrapper: ```cpp -// operator= (struct copy) — generates lwz/stw (word load/store): -// lwz r0, 0(rSrc); stw r0, 0(rDst) -// lwz r0, 4(rSrc); stw r0, 4(rDst) -// lwz r0, 8(rSrc); stw r0, 8(rDst) -node.mPos = param_1; - -// .set(vec) (float copy) — generates lfs/stfs (float load/store): -// lfs f0, 0(rSrc); stfs f0, 0(rDst) -// lfs f0, 4(rSrc); stfs f0, 4(rDst) -// lfs f0, 8(rSrc); stfs f0, 8(rDst) -node.mPos.set(param_1); - -// .set(x, y, z) (3-arg form) — generates lfs/stfs like component assignment -node.mPos.set(expr_x, expr_y, expr_z); +DSPInterface::setPauseFlag(channel->unk0, 1); +DSPInterface::flushChannel(channel->unk0); ``` -The target assembly will clearly show `lwz`/`stw` (integer move) vs `lfs`/`stfs` (float move). Choose the source pattern that matches. +Both forms give the same instructions, but the wrapper form reads correctly and gives correct registers in long functions. +Case: `JAInter::StreamLib::callBack` went from 98.7% to 99.8%. +Keep the pointer local only where the code reads a field through it. -### Constructor with a zero component + `+=` vs direct component init +### Look at the out-of-line copies before you change an inline + +When MWCC does not inline something, it emits a weak or local copy into the caller's TU, and `mario.MAP` lists it. +That copy is the original body. Diff it before you change the inline. +If the ROM calls an inline out of line at some sites and expands it at others, the difference is at those sites, not in its body. + +The number of `blr` instructions in a weak copy tells you the shape of the body. +Case: the weak copy of `TSpineBase::getLatestNerve` has two `blr` and no join, so it is `if (mCurrent) return mCurrent; return mPrevious;`, not a ternary. +A ternary would make it `CI_CanInline6` and change every inline decision around it. + +### `-inline deferred` and the order of definitions + +When a TU uses `-inline deferred` (most game code), define the functions in reverse order of the map. +The `src/JSystem/JAudio` TUs do **not** use it: their order follows the map forward. +Without `-inline deferred`, MWCC can inline a helper only into a function defined later in the file. +Thus, only the symbols below an UNUSED function in the map can be its callers. + +## Loads, stores and CSE + +### MWCC does not reorder + +MWCC never moves memory stores, loads and calls relative to each other. +Write them in the source in the same order as in the target. +This is a good first thing to fix, because it gives shape to the rest of the function. + +Any call removes all cached loads. +Most stores through a pointer do the same. +Use this to find where the original code loaded and loaded again. + +### MWCC removes redundant reads, never writes + +Each write in the source occurs in the assembly. +Thus, when the assembly stores the same location twice, the source also wrote it twice. + +MWCC can remove a redundant read, but not always: an inline expansion can prevent it. + +### Where member-load CSE happens + +The front end merges repeated reads of `this->member` (`frontend-01` already has one load). +Inlining is complete before this optimizer runs, so all accessor calls look the same at that time. +These things stop the merge (`IRO_GetExprKills` in `IroCSE.c`): + +- a call, +- a store through a pointer (an indirect store). + +A store to a plain local does not stop it. + +A member at **offset 0** is not a CSE candidate: its address is the bare `this`, not `this + offset`. + +### An inline accessor breaks member CSE + +The merge follows the **spelling**, not the address. +A read inside an inlined accessor is bound to that call's temp, so a later direct read of the same member is a **second load**. +An extra `lwz` of a member that MWCC could keep in a register is evidence of an inline accessor. + +Case: `SDLModel::entry()`. The target loads `mSdlFlags`, tests two bits, then loads it again for `mSdlFlags |= FLAG_UNK1`. +`if (!(mSdlFlags & FLAG_UNK8) || ...)` gave one load (87.5%). +`if (!checkSdlFlag(FLAG_UNK8) || ...)` with `u32 checkSdlFlag(u32 flag) const { return mSdlFlags & flag; }` gave the target (96.3%). + +- Do not return `bool` from such a checker. In a `||` chain, a `bool` return makes MWCC compute 0 or 1 and then test it. Return the masked bits. +- A `void` accessor on `this` (an "on" or "off" setter) costs no temp and changes no instruction. + +For a flag word that a function tests many times and clears in the taken branches, the ROM shape is one `lhz` for the `if`/`else if` chain and a new `lhz` before each `offFlag()`. +You get this only when **every** read goes through the accessor. +One raw `mFlags & X` in the chain becomes the CSE root, and the loads again disappear. + +The mask width is a different effect. +`!checkFlag(~(top bits))` gives `clrlwi. r0, rN, 19` only when a `u16` is in the signature (parameter or return). +Prefer `BOOL checkFlag(u16 flag) const`. + +`JDrama::TFlagT::check(mask)` computes `(mValue & mask) != 0`, so the compare is `cmpwi`. +A direct `f.mValue != 0` gives `cmplwi`. +An `lhz` followed by `cmpwi rN, 0` proves `check()`. Take the mask from the `on()`/`off()` call on the same field. +Write 0 back with `f = 0`: `off(0xffff)` is a read-modify-write. + +### `const` on an inline's pointer parameter stops CSE + +When an inline reads a field through a pointer parameter and the caller reads the same field, MWCC normally merges the two loads. +A pointer to `const` stops the merge: -When building a vector where some components are `0.0f + ` and one is a -real value, the target may show *literal* `0.0` adds on the zero components: -``` -lfs f4, @zero ; fadds f4, f4, f0 ; component = 0.0 + src.x -``` -This is the signature of the original writing a base vector with literal-zero -components and then adding another vector, e.g. ```cpp -// point2 = (0, -unkC.y, 0) then add unk58 -> emits the "0.0 + unk58.x" adds -JGeometry::TVec3 point2(0.0f, -1.0f * unkC.y, 0.0f); -point2 += data.unk58; +const char* getSymbolName(TSpcSymbol* symbol); // one shared load +const char* getSymbolName(const TSpcSymbol* symbol); // two loads ``` -Writing the fully-fused form `TVec3(unk58.x, -unkC.y + unk58.y, unk58.z)` -instead reads the components directly with no zero-adds and does not match. -Concrete case (`BathWaterManager.cpp` `calcBathtub` inner-band branch): the -`+=` form recovered the `0.0 + x`/`0.0 + z` adds; see next tip for the `-1.0f *`. -### Negation as `-1.0f * x` (fmuls) vs `-x` (fneg) +Case: `Strategic/spcinterp.cpp` `TSpcInterp::dump` went from 98.8% to 100%, and every other caller still matched. +Try this before you restructure anything. -`-x` compiles to `fneg`. But the target sometimes shows a standalone -`fmuls f_, f(-1.0), f(x)` (multiply by the `-1.0` SDA constant) where you'd -expect a negation — and crucially kept as a *separate* fmuls, not fused into a -following `fmadds`/`fnmsubs`. This happens when the original literally wrote -`-1.0f * x` (MWCC in does not algebraically fold -`-1.0f * x` -> `fneg x`). Prefer `-1.0f * x` over `-x` when the target shows the -extra `-1.0` load + `fmuls`. Concrete case (`BathWaterManager.cpp` -`calcBathtub`): `-1.0f * unkC.y` matched; `-unkC.y` gave `fneg` and did not. +### Reading a source array through `void*` stops CSE -### Trouble in fnmsubs town - -Computation-heavy code commonly has code like `B - A * C` or `A * C - B` for floating-point A, B, C. E.g. any cross product will have analogous code. -MWCC likes to compile such code into PowerPC `fmsubs` and `fnmsubs` instructions, but they have a quirk in how they are implemented related to double to single precision conversions: technically, fnmsubs is `-float(A * C - B)`, but MWCC uses it for `B - A * C`. -Decompilers like ghidra and m2c like to preserve this technicality and commonly emit code like `-(a * b - c * d)` in code like vector cross products. -Rewriting it as `c * d - a * b`, opening the braces, usually helps matching it better. - -## Reading a source array through `void*` defeats CSE (controls loop unroll) - -When a loop copies the same source element to several destinations, e.g. ```cpp TVec3* src = (TVec3*)getVtxPosArray(); for (...) { a[i] = src[i]; b[i] = src[i]; c[i] = src[i]; } ``` -MWCC's alias analysis can prove `src` (a clean typed pointer) doesn't alias the -`new`-allocated destinations and **CSEs the `src[i]` load** — reading it once and -reusing it for all three stores. That shrinks the loop body, and MWCC's `-O4` -unroller then unrolls it *more* (e.g. ×8). The target often reloads `src[i]` for -every store (no CSE) and unrolls only ×2. -To reproduce the reloads (and the smaller unroll factor), read the source -through a `void*` and cast at each use, which blocks the alias analysis: +MWCC can prove that `src` does not alias the destinations, so it loads `src[i]` once. +The smaller loop body then unrolls more (×8). +When the target loads `src[i]` for each store and unrolls ×2, read through a `void*` and cast at each use: + ```cpp void* src = getVtxPosArray(); for (...) { @@ -572,162 +543,127 @@ for (...) { c[i] = ((TVec3*)src)[i]; } ``` -Concrete case (`DrawUtil.cpp` `TTrembleModelEffect::init`): the typed-`src` form -CSE'd + unrolled ×8 (54%); the `void*`-cast form reloaded + unrolled ×2 (100%). -## `const` on an inline's pointer parameter also defeats CSE +Case: `DrawUtil.cpp` `TTrembleModelEffect::init` went from 54% to 100%. -The same effect appears at an inline boundary, and there the fix is one word. -When an inline reads a field through a pointer parameter and the caller reads -the *same* field of the *same* object, MWCC normally merges the two into one -load. Making the parameter point to `const` stops the merge, and both loads -appear. +### A global base against a local base -```cpp -// one load of symbol->mNameOffset, shared with the caller -const char* getSymbolName(TSpcSymbol* symbol); -// two loads: the inline's own, and the caller's -const char* getSymbolName(const TSpcSymbol* symbol); +`IRO_IsSubableExpression` (`IroSubable.c`) rejects `base + constant` only when the base is a local that can live in a register. +A global base is always a CSE candidate, and gets one address temp for each constant that the code uses twice or more. + +- ROM: `lwz rX, 0xNN(rBase)` everywhere. Ours: `addi rT, rBase, 0xNN` and `lwz rX, 0(rT)`. The ROM base was a local or a pointer parameter, and ours is the global. +- The reverse means that you added a local that the original did not have. + +How you seed the local matters: + +- `T& x = TheGlobal;` and `T* x = &TheGlobal;` propagate back into the global, and the temps come back. +- `T* x = GetTheGlobal();` (a getter) survives propagation. +- An `inline` helper whose parameter is the pointer also works, when the argument is `GetTheGlobalPtr()`, not `&TheGlobalObj`. + +Every source-declared register local ranks above `this` in the allocator. +If the target keeps `this` in r31, the original had no declared register local there, and the temps have another cause. + +### A member function called on a global: bare name against accessor + +On a global written by name, `gpApplication.setNextArea(x)`, MWCC puts the constant address in place of `this`, folds the member offsets, and makes `gpApplication + 0x12` a CSE temp: + +``` +addi r30, r5, 0x12 ; &gpApplication.mNextArea +stb r0, 0x12(r5) +... +addi r3, r30, 2 ; &mNextArea.unk2 ``` -Concrete case (`Strategic/spcinterp.cpp` `TSpcInterp::dump`): the original loads -`symbol->mNameOffset` twice, once inside the inlined `getSymbolName` and once as -a `SpcTrace` argument. Adding `const` took `dump` from 98.8% to 100%, and left -every other caller of `getSymbolName` matching. +When the receiver is an expression that MWCC does not repeat (an inline accessor call, a local pointer, a local reference), `this` is bound to a temp that holds `&gpApplication`: -This is worth reaching for before restructuring anything: it is a smaller and -far more plausible change than the usual alternatives (hoisting the field into a -local, or dropping the intermediate local altogether), both of which were tried -here and were worse. - -## An inline's locals are numbered in reverse when it is inlined - -This one explains a whole family of "the out-of-line copy and the inlined copy -want opposite stack layouts" puzzles, so it is worth knowing the mechanism. - -`CParser_NewLocalDataObject` **prepends** each new local to the function's -`locals` list, so the list is in reverse declaration order. Two consumers then -walk that list forward: - -- `assign_locals_to_memory(locals)` gives out stack offsets, **increasing** — - so when a function is compiled normally, the **first-declared** local ends up - at the **highest** offset. -- `CInline_SetupArgsExpression` recreates the callee's locals in the caller, - walking the packed array forward and prepending each one — which **reverses - the order a second time**. So in an inlined copy the **last-declared** local - ends up at the highest offset. - -The consequence: any two named locals of an inline function **swap places** -between its out-of-line copy and every site that inlines it. No declaration -order satisfies both. Measured on `TSpcInterp::fetchU32`: `src` first gives the -four inline sites 100% and the out-of-line copy 99.8%; `result` first gives -exactly the reverse. - -The escape is not to reorder but to **reduce the function to a single local -that needs a stack home** — with nothing to swap, both layouts agree. A local -whose initialiser carries a type-conversion node gets propagated into a compiler -temporary and stops needing a home, so writing the accessor to return `void*` -and casting at the call site is enough: - -```cpp -void* getText(u32 offset); // was u8* -u8* src = (u8*)mBinary->getText(mProgramCounter); // now a temp, not a local -u32 result; // the only named local left +``` +addi r31, r4, gpApplication@l +stb r0, 0x12(r31) +stb r0, 0x13(r31) +addi r3, r31, 0x14 ``` -That took `fetchU32`, `fetchS32`, `execvar` and `execfunc` to 100% at once -(`spcinterp` 62.2% -> 67.7% matched code). Note it also fixed the two call sites -that fetch *twice*: a second named local was what pushed their temporaries out -of step, not anything about the second fetch itself. +The second shape is the sign of an accessor such as `SMSGetApplication()`, `SMSGetMSound()` or `SMSGetMarDirector()`. +The two forms differ only where the result goes into another inline call. -Symptom to recognise: the out-of-line copy of a weak inline and its inline sites -each want the same pair of slots in opposite orders, and both frames are already -the right size. +The same test tells a getter from a raw field read. +A getter such as `u8 getStage() const { return unk0; }` forces its value into a temp. +The temp keeps a 4-byte slot, and when two such calls are the arguments of one `bl`, MWCC evaluates them in reverse order. +Case: `TMarDirector::currentStateFinalize` loads `mCurrArea.unk1` before `unk0` for `endStageEntranceDemo`; only the getters do this. +An accessor that returns a reference leaves no trace, so you cannot prove or refute it this way. -## `T x = f();` costs one more stack object than `T x; x = f();` +### Array element reads -When `f` is inlined, initialising a local from the call builds the return value -in its own temporary and then copies it into `x` — **two** stack objects. -Declaring first and assigning lands the inlined return straight in `x`'s slot — -**one**. Neither spelling changes a single instruction; only the frame moves, so -this is the cheapest knob there is when a frame is off by exactly the size of -one object. +For `mArray[index]` where `mArray` is a pointer member: + +| Reads in the block | Code | +|---|---| +| 1, into a named local | `lwzx r0, base, idx`, then `mr` into the argument register | +| 2 | `lwzx r4, base, idx`, one load, straight into the register that needs it | +| 3 | `add r3, base, idx` and `lwz r4, 0(r3)`: MWCC also keeps the address | + +A named local gets a scratch register, but a CSE is put in the register that its user wants. +For "test it, test it against a sentinel, then pass it", use a local for the two tests and a new read for the argument. + +### An address held across calls + +When the target holds one element address in a callee-saved register across calls, name the **array base**, not the element: ```cpp -ExecFunction f = chooseExecFunction(cmd); // return temp + copy -ExecFunction f; f = chooseExecFunction(cmd); // return lands in f +FabricatedSeTrackParameter* seTrackUpdate = basic->unk0->unk0; +seTrackUpdate[index].mVolume; seTrackUpdate[index].mPan; ... ``` -Both directions have paid off in `Strategic/spcinterp.cpp`: -- `TSpcInterp::update` was 8 bytes too big holding an inlined 12-byte - pointer-to-member. Declare-then-assign took it to 100%. (Binding a - `const T&` instead is in between — it drops the copy but adds an address - register: 96.2%.) -- `TSpcBinary::init` was 8 bytes too *small*. Spelling the inlined `calcKey` - result as a local — `u32 hash = calcKey(...); symbol->mNameHash = hash;` - instead of assigning the call directly — added exactly the two missing - objects and took it to 100%, with `calcAndStoreKeys` still size-exact - against the map. +`seTrackUpdate + index * 0x18` uses only locals, so no call can remove it. +MWCC keeps it in a compiler temp, and a temp ranks above every named local. +Case: `JAIBasic::setParameterSeqSync` went from 95.0% to 99.9%. +Do not use this when a call is between the uses of a value read from memory. That is a real local. -So: frame one object too big, look for an initialisation to split; one object -too small, look for a call result that should have been named. +### A pointer local read twice in one block -## Reading a value into a local removes a bound temporary +`IRO_ExpressionPropagation` copies an assignment into its reader only when the variable has exactly one read in the same basic block. +A local that survives folds a member offset into the load displacement. +A local that is propagated adds the offset to the index. -`CInline_SetupArgsExpression` binds an argument to a compiler temporary whenever -the expression is unsafe to repeat — and a call is always unsafe. A read of an -unmodified local is safe, so it gets substituted instead and **no temporary is -created**. When a frame holds one 4-byte temporary too many, moving the argument -into a local is often the whole fix: - -```cpp -interp->push((int)interp->pop().typeof()); // binds a temp for the argument -u32 type = interp->pop().typeof(); // no temp; `type` stays in a register -interp->push((int)type); +``` +add rB, rBase, rIndex ; the original read the pointer two or more times +lwz rB, 4(rB) ``` -Concrete case (`Strategic/spcinterp.cpp` `spcTypeof`): 96.3% to 100%. +If our build gives `addi rI, rIndex, 4` and `lwzx`, write the member access out again at the second use. +CSE merges the two loads, so the second read costs no instruction. +Case: both loops of `J3DDeformer::deform`. -Beware the mirror image: this only helps when the local itself does not need a -slot. If the enclosing function already has a settled set of named locals, the -new local lands in the local region and shifts every offset above it, which -costs more than the temporary saved. The same change in `execadd` dropped it -from 100.0% to 96.6% for exactly that reason. +### Repeated vector components: the offset-0 field -## Each inline level in a call chain leaves one dead 4-byte temporary +When one object gives `x`, `y` and `z` to two stores in sequence, front-end CSE makes one temp for `y` and `z` but two for `x`. +`x` is at offset 0, a bare object reference. +The second `x` temp carries the value, so `x` is coloured after `y` and `z`: `x=f2, y=f0, z=f1` where the original has `f0, f1, f2`. +Access through a pointer or reference makes `x` an offset too, but it keeps the address live and can rotate the GPRs. -An expression such as `gpFoo->getBar()->baz()` builds one compiler temporary per -inline level that it goes through, and each temporary keeps a stack slot even -when the optimiser removes every instruction that touched it. -The instruction stream is then identical to the ROM's and only the stack offsets -disagree — the classic "our frame is 8 bytes short" symptom. +A target that loads three times and stores six times proves a hidden copy that is not address-taken, for example the temp of an implicit `Vec` to `TVec3` conversion. -Two levers add a level, and both are ordinary source that a person would write: +### Local structs stay in registers only when you touch them field by field -- **Go through the global's inline accessor.** - `SMSGetMarDirector()->getConsole()` instead of `gpMarDirector->getConsole()` - adds exactly one temporary. This only works when the *next* member call is - itself inline; if the accessor result feeds an out-of-line `bl` at once, MWCC - reads the global in place and no temporary appears. -- **Hold the chain result in a named local.** - `TGCConsole2* console = SMSGetMarDirector()->getConsole(); console->foo();` - adds one more temporary than the same expression written in one line, because - the local is used as the receiver of the following call. +`IRO_ScalarizeClassDataMembers` (`IroVars.c`) gives each field access of a local struct, class or array its own temp, so it can live in a register. +An access with an unknown offset, or one that overlaps a field with a different type or size, puts the **whole** variable in memory. +A whole-struct copy (`*(Vec*)this = *(Vec*)&other`, a 12-byte struct copy) overlaps every field. +So MWCC never keeps a `TVec3` half in registers and half on the stack. -Concrete case (`System/EventWatcher.cpp` `evManiCoinDown`): the bare -`gpMarDirector->getConsole()->startAppearStar()` gave 2 temporaries and a 0x28 -frame; the accessor gave 3 and 0x30 with the wrong layout; the accessor plus the -`console` local gave 4 and a 100% match. +A cast between the dereference and the object reference also puts the local in memory. +After a call, MWCC must reload every field of such a local. +The signature of the ROM `TVec3` copy: `local.y` and `local.z` stay in `f31`/`f30` across calls, and `local.x` (offset 0) is loaded again. -Because the effect depends on what follows the accessor, do not convert a whole -file blindly — measure each site. In the same file `evSetNextStage` is -measurably *worse* with the accessor, which is real evidence that the original -used the bare global there. +### MWCC does not optimize data that it stores as ints -## A by-value class parameter forces the copy through memory +When the compiler-generated copy of a `Vec` (3 floats) runs, it uses integer loads and stores. +MWCC then cannot keep the values in float registers and puts them on the stack. -When a class is passed by value, MWCC materialises the copy and reads the copy -back, even for a two-word POD: +When you set the four bytes of a `Color` one by one, MWCC cannot merge them into bit operations in a register, and keeps the struct on the stack. + +### By-value class parameters and copy constructors + +A class passed by value is copied through memory, also for a two-word POD: ``` lwz r4, 0(r3) ; source.mType stays in a register @@ -739,68 +675,975 @@ stw r0, 0x40(r1) ; copy.mData lwz r0, 0x3c(r1) ; the copy is read back from memory ``` -Two stack objects, and the second field never gets forwarded through the -register. Seeing this shape in the target is a reliable sign that the value goes -into a by-value parameter — in `EventWatcher.cpp` that parameter is -`getNameRefPtr(TSpcSlice)`. A named local initialised from the same call gives -two objects as well, but forwards the first field through a register, so the two -shapes are easy to tell apart. +This shape in the target is a reliable sign of a by-value parameter (in `EventWatcher.cpp`, `getNameRefPtr(TSpcSlice)`). +A named local initialized from the same call also gives two objects, but it moves the first field through a register. -## Working with JSUMemoryInputStream +For a by-value class argument to a function that MWCC does **not** inline: -Practice shows that most of the time in game code `operator>>` overloads were used for reading from them, and sometimes they were chained together. -This fact was derived from stack frame padding issues in various `JDRama::TNameRef::load` overloads -- only by using `operator>>` and sometimes chaining them (e.g. `stream >> vec.x >> vec.y >> vec.z;`) does stack the stack frame size matches the original. -However, methods like `stream.readU16()` were also occasionally used, but codegen differs a bit when it is used and so judgement must be made on a case-by-case basis. -Doing raw `stream.read(&someInt, 4);` calls is the least likely option, because humans are lazy and wouldn't want to specify the size manually -- always prefer avoiding it. +- With a **user-declared** copy constructor, MWCC builds the argument in place: one store, no copy. +- With the **implicit** copy constructor, MWCC copies through a separate stack temp: `lwz`/`stw` pairs from slot to slot. -## Forcing local arrays onto the stack with unrolled loops +So stack-to-stack word copies around a small value class mean that the class has **no** user-declared copy constructor. +The weak `__ct__FRC` symbol in the map does not prove a declared copy constructor. +Case: deleting the hand-written copy constructor of `TSingleNodeLinkList::iterator` took `TPerformList::perform` from 41.8% to 99.1%. -Small local arrays (e.g. `f32 dist[3]`, `f32 m[3][2]`) as well as structures are prime candidates for MWCC's *scalar replacement of aggregates* — it promotes the elements into FP or GP registers, drops the stack storage entirely, and is then free to reorder/fuse the element computations. -If the target keeps such an array on the stack (you see `stfs`/`lfs` of consecutive slots, computed in a strict order), your straight-line, element-by-element source will usually mismatch because yours got scalarized. +For `JUtility::TColor`: -Two ways to force the array back onto the stack: +| Callee parameter | Argument | Code | +|---|---|---| +| `TColor` by value | `TColor(r,g,b,a)` or a default argument | 4 `stb`, `lwz`, `stw` to the parameter, `lwz`: one copy too many | +| `const TColor&` | either | no copy, but the frame is 8-16 bytes short | +| `u32` | `TColor(r,g,b,a)` written at the call site | 4 `stb`, `lwz` into the argument register: exact | +| `u32` | `TColor(u32Member)` written at the call site | `lwz; stw; lwz`: exact | +| `u32` | a `TColor` member through `operator u32()` | one `lwz` | -1. `(void)&arr;` — taking the address marks it address-taken so it must live in - memory. Works, but it's an obviously-fake artifact. - Still useful as an intermediate step in figuring out the other details of the function to then replace with a proper match. -2. **Write the init/compute as a `for` loop that MWCC unrolls.** Iterating over - the array with an index the compiler can fully unroll (small constant trip - count) makes MWCC materialize the array on the stack and emit the element - stores/loads in loop order — no address-of hack, and it reads like plausible - original code. This also tends to fix the *ordering* of the stores, not just - their presence. +`JUtility::TColor` has **no** destructor. +`TSmplFader`'s constructor takes `TColor` by value and is inlined at every site, which `CI_CanInline0` forbids for a class with a destructor. +Adding `~TColor() {}` was measured: 82 functions worse, 1 better. Do not try it again. + +### Triviality: user-declared destructors + +A user-declared destructor (also an empty `~T() {}`) makes a class non-trivial. +MWCC keeps non-trivial types in memory, and the property goes up through members and bases. + +Two signs of a **missing** destructor (the target keeps a value in memory, ours keeps it in a register): + +- The target reloads struct fields after calls, but ours keeps them in registers. +- The target writes a new `new`-ed object pointer to the stack and reloads it as `this` during an inlined constructor. + For `new T()` with a non-trivial `T`, the object is a separate address-taken temp. + +Give the smallest value type an empty `~T() {}` and measure again with a full baseline: one destructor can change many sites. +Case: `~TMsRange()` (a field of `TSmallEnemyParams`) took several `TFooManager::load` functions from about 95% to 100%. + +The reverse also occurs: a destructor that the original did not have breaks constructors. +Case: removing `~JAIBasic();` (no definition, and no `__dt__8JAIBasicFv` in the map, not even UNUSED) took the `MSound` constructor from 90.22% to 99.78%. +When a base class declares a destructor that the map does not list, remove the declaration and measure. + +Every destructor has a hidden `s16` delete flag. +The generated tail tests `this != 0` and `flag > 0`, and inlines `operator delete`. +An empty destructor is therefore about 0x70 bytes, not 4. +Subtract this tail before you size an UNUSED destructor body. + +### Chained assignment of two locals ```cpp -// scalarized into registers + reordered (mismatch), or needs (void)&: -f32 dist[3]; -dist[0] = unk24[0]; dist[1] = unk24[1]; dist[2] = unk24[2]; +f32 y = pos.y; // lfs fT, pos.y +f32 g = y; // fmr f20, fT + // fmr f21, fT <- a temp and TWO copies -// unrolled loop: array is spilled to the stack in order (match): +f32 y; +f32 g = y = pos.y; // lfs f20, pos.y + // fmr f21, f20 <- one copy +``` + +In the first form, copy propagation changes `g = y` into `g = pos.y`, so the load gets its own temp and two copies. +The chained assignment makes `g` a copy of the **variable**, which propagation does not change. +Look for a load into a callee-saved register, followed by exactly one register move, with both registers live after it. + +### A local in memory is not a spill + +A value in a stack slot, loaded again at each use, is almost never a spill. +MWCC spills only when it runs out of 18 callee-saved registers. +Check the `spills:` section of `variables.txt` before you believe a spill. + +These put a local in memory: + +- any use of its address (`&x`), +- a struct or union type (`assign_register_by_type` gives registers only to int, enum, pointer and float), +- a sub-word type `u8` or `u16` for a named local in some cases (`u32` gets a register). + +A load of a memory local is still hoisted out of a loop, because a store through an unrelated pointer does not alias a local. +To keep the load in the loop, use a struct or union local. +A one-byte local whose address escapes does not stop the hoist; a 4-byte array or a struct does. + +### Do not use reinterpret casts to access fields + +Never access a field through a raw byte offset and a cast. +The codegen almost always differs from a real field access. +Add the field to the class, or use the existing getter or setter. + +## Register allocation + +### How MWCC colours registers + +`allocate_local_vregs` (`CodeGen.c`) numbers the vregs in this order: + +1. arguments, in parameter order, +2. named locals, in **reverse** declaration order (`CParser_NewLocalDataObject` prepends each new local), +3. front-end temps `@NNN` (a temp made later gets a lower number), +4. back-end temps, +5. the TOC list. + +`simplifygraph` (`Coloring.c`) sweeps the nodes in **ascending** number. +It pushes each node whose degree is below the number of available registers (29 for GPRs: `r0`, `r3`-`r12`, `r14`-`r31`), and decrements the degree of each neighbour. +It sweeps again until a sweep pushes nothing. +`colorgraph` pops the stack, so MWCC colours the nodes in **reverse push order**. +Each node takes the lowest free volatile register, or an already used callee-saved register. +Only if none is free does it take a **new** callee-saved register, counting down from `r31`. + +The rules that follow: + +- **The first-declared named local gets the highest callee-saved register.** +- **Every compiler temp outranks every named local.** Arguments rank below locals. +- A local in an inner scope ranks above the locals of the outer scope. +- `r0` goes to the higher-numbered node of an interfering pair. A load or store base can never be `r0`. + +When a value is coloured earlier than its number says, it was not pushed in the first sweep. +This is the "degree limit" case below. + +### Local declaration order picks the callee-saved registers + +When the instructions match but the callee-saved block is rotated, permute the **declaration order** of the locals, not the code. +Declaring a local earlier moves it **higher** (towards `r31`). +Do the "move each declaration to the front" sweep first. It usually finds the responsible variable in one pass. +Case: `J3DModel::calcWeightEnvelopeMtx` went from 97.2% to 100% with `u8* pScale` moved from last to after `int max`. +When several positions give 100%, choose the one that reads most naturally. + +A target that holds a loop-invariant value **below** a named local tells you that the original named it, and declared it after that local. +Case: `JASystem::Driver::updateMixer` went from 99.3% to 100% with `f32 vol; f32 invPan = 1.0f - pan;`. + +### Inside an inline expansion the order flips + +In an inline expansion, MWCC prepends the callee's locals a second time. +So the **last**-declared local of an inlined body gets the highest register, and the highest stack slot. + +| Body of the inlined helper | Registers | +|---|---| +| `s32 uVar1 = 1; JKRSolidHeap* rootHeap = heap;` | `rootHeap` r31, `uVar1` r30 | +| `JKRSolidHeap* rootHeap = heap; s32 uVar1 = 1;` | `uVar1` r31, `rootHeap` r30 | + +Thus, two named locals of an inline swap places between its out-of-line copy and each inlined site. +No declaration order satisfies both. +The fix is to reduce the function to **one** local that needs a stack home. +A local whose initializer has a type conversion becomes a compiler temp: + +```cpp +void* getText(u32 offset); // was u8* +u8* src = (u8*)mBinary->getText(mProgramCounter); // now a temp, not a local +u32 result; // the only named local left +``` + +Case: this took `TSpcInterp::fetchU32`, `fetchS32`, `execvar` and `execfunc` to 100% at once. +The signature: the out-of-line copy and the inline sites want the same pair of slots in opposite orders, and both frames are already the right size. + +Arguments of an inlined function still rank below its locals and stay in parameter order. +A target that holds the **first** parameter above the second cannot do that with bare parameters: the original put one of them in a local. + +### Bound arguments run last parameter first + +`CInline_SetupArgsExpression` binds each argument that it cannot repeat safely (a call, a forced getter load, a member read of an address-taken local) to a temp. +Each new binding becomes the left operand of a comma chain, so the bindings execute **last parameter first**. +The last parameter's temp gets the highest stack slot and the highest-numbered register. +Constants, unmodified locals and reads of `const` objects are not bound: they are evaluated where the body uses them. + +When the ROM evaluates two complex arguments of an inlined function in source order, the parameter list is the other way round. +Case: `JDrama::TOrthoProj`'s constructor is `(near, far, top, bottom, left, right)`, because the ROM computes `rect.getWidth()` before `rect.getHeight()`. + +### The first web and later webs + +When exactly **one** register differs and every other instruction matches, a value was coloured at the wrong point in the priority order. +It took a register that something else had already used, instead of a new one. + +MWCC splits a variable into webs. +The **first** web keeps the variable's own low number. +**Later** webs become temps with high numbers, so they go far up the priority order. + +- To lower the priority of a value, do not reuse a variable that is live before. Give it its own variable. +- To raise its priority, reuse a variable. + +Neither change adds an instruction, because the extra local coalesces away. +Diagnosis: find the vreg in `backend-*-before-regalloc.txt`, and look at its neighbours. +If the vreg does **not** list the register that the target used, that register was free: this is a priority problem, not an interference problem. +Case: `J2DWindow::draw_private` became exact with a fifth `u16` for the second edge block. + +### When CSE temps are born + +`IRO_CommonSubs` makes the temp **at the second occurrence** of the expression, not at the first and not at the last. +An address that the code reads at the top of a function and again in the last statement is born last, gets the lowest number, and is coloured last. +The outermost matching expression wins: a repeated `*(addr)` hides the repeated `addr` under it. +A plain read of a local that can live in a register is never a CSE temp itself, and `local + constant` is not either. + +To give a value a low priority, keep it a plain named local, and do not read it again in the same block before the first call. + +The optimizer pass that makes a temp sets its number, not its position in the AST. +Statement order is a real lever; operand order inside one statement is not. + +### The degree limit: `this` in r31 instead of r28 + +When the instructions match but `this` sits in `r31` instead of `r28`, the cause is the **degree** of `this`. +`this` is live for the whole function, so it interferes with every vreg and with 12 physical registers: + + deg(this) = 12 + (number of live virtual registers) + +At 16 vregs the degree is 28: `this` is pushed first and coloured last, and gets `r28`. +At 17 vregs the degree is 29: `this` waits for a later sweep, is coloured first, and gets `r31`. +Everything else moves by one. + +In `regalloc-gpr-pass-1-all.txt`, the signature is `this` at the **top** of the assigned list with `neighbors: 30`. + +To find the cause fast, make a variant that is exactly one vreg over the limit. +Then each rewrite that removes one vreg flips `this` back to `r28` at once. + +Rewrites that each remove one vreg: + +1. A shared `u16` value used in two places gets a `u16` CSE temp and a redundant `rlwinm`. Cast **both** uses to `(u32)`. +2. `var = call();` makes `mr tmp, r3` then `mr var, tmp`. `var += call();` (with `var` set to 0 before) folds the copy. + +The opposite lever: **a reused local adds one vreg per assignment**, because each assignment starts a new web. +Case: `JAInter::StreamLib::Play_DirectPCM`. Twelve `getDSPHandle(...)->setter()` expressions through one reused `pDVar1` put `channel` in `r31`, and took the function from 81.6% to 100%. + +### Coalesced nodes raise every neighbour's degree + +The sweep skips coalesced nodes. +A coalesced node is never pushed, so it never decrements its neighbours: it adds +1 to the degree of each node that it interferes with, for the whole colouring. + +These make a coalesced node: + +- a named local that goes straight into a call argument (`mr r4, vN` coalesces `vN` into `r4`), +- a value that a load puts in a temp and that then goes into an argument register, for example an inline getter's forced load or the bound `this` of an inline wrapper. + +These make no node: + +- a value that MWCC writes directly into the argument register, +- an address argument (`&this->field`), which propagation removes. + +When the colouring stalls (a full sweep pushes nothing), MWCC pushes the node with the lowest `spillCost / degree`. +That node is coloured **last** and takes the **lowest** callee-saved register. +This is the only way a node gets a register below `this`. +Loop counters have a high spill cost. The jump-table base and the division magic number in a switch loop have a low cost. + +### Replay the allocator when only registers differ + +When a function has no opcode or order differences, only register names, the interference graph has the same shape as the target. +Only the node set can differ: one node too many or too few. +Guessing at source shapes is slow. Replay the allocator: + +1. Dump the graph with the MWCC debugger. +2. Reproduce `simplifygraph` and `colorgraph` in about 90 lines of Python. Check that the replay gives our build's registers exactly. +3. Search: delete each node, un-coalesce each coalesced node, merge each pair, or add k coalesced nodes next to `this`. Compare each result with the target's registers. +4. The edits that work name the source change. + +Cases: + +- `JASystem::BankMgr::noteOn`: our graph had one node too many. The fix was one variable written twice (see below). +- `TDrawSyncManager::start` (98.2% to 100%): every solution was "one more neighbour of `param_2`". A named local used while `param_2` is live (`OSMessage* msgs = new OSMessage[0x14];`) fixed it. +- `CPolarSubCamera::ctrlJetCoasterCamera_` (98.3% to 99.8%): `this` needed 4 more coalesced neighbours. Real global accessors gave them: `TFlagManager::getInstance()` twice, `getConsole()`, `SMSGetMarDirector()`. + +When `this` must move up to `r31`, count how many nodes are missing. +Then look for global pointer calls (`gpX->f()`, `smInstance->f()`) where a real accessor adds a node with no instruction change. + +### One value written twice into one variable is one node + +```cpp +chanKey |= instParam.unk3C << 0x10; // 2 nodes: the load temp and the shift temp +u32 keymap = instParam.unk3C; // 1 node: the local is both destinations +keymap <<= 0x10; +chanKey |= keymap; +``` + +`u32 keymap = instParam.unk3C << 0x10;` still makes two nodes. +The sign in the target: a load and its arithmetic in **one** register. + +`X* p = call();` makes `mr tmp, r3` then `mr p, tmp`. + +- If `p` is only a load or store base, propagation removes it. +- If `p` ever goes into an argument register, it keeps its node, and the return temp becomes a coalesced node. + +No spelling of `X* p = call(); if (!p) ...; p->method();` reduces this. Search elsewhere. + +### A local that feeds a call argument moves its computation earlier + +MWCC sets up a call as `mr r3, `, `mr r4, `, and then computes an argument expression straight into its register. +The temps of that expression are live across the moves, so they can never get `r3` or `r4`. +A named local moves the computation in front of the moves. + +``` + target ours + mulli r3, r0, 0xc mulli r5, r0, 0xc + addi r5, r3, 4 addi r3, this, 0 + addi r3, this, 0 addi r5, r5, 4 +``` + +The target uses `r3` for the multiply, so the value was a local. + +An inline accessor in argument position evaluates its index before the base. +Case: `SDLModel::entryModelDataSDL`. `getShapePacket(idx)` in place of `&mShapePackets[idx]` gave the target's load order and all its callee-saved registers. +But an accessor in argument position can also move a load before the register setup: `load(..., getTextureResource())` dropped `JPAResourceManager::load` from 99.8% to 85.2%. +When the target loads the arguments in order, put the extra level on the receiver chain or inside the callee. + +### Constant locals + +Constant propagation puts an immediate in place of a constant local only where the operand can be an immediate: + +```cpp +int emitterNum = 0x100; +new JPAEmitterManager(res, particleNum, emitterNum, emitterNum * 2, nullptr); +// li r28, 0x100 ; slwi r7, r28, 1 ; li r6, 0x100 +``` + +An `li rN, K` next to `slwi rM, rV, n`, where `rV` also holds `K`, is **one** variable used twice. + +Two counters declared once at **function scope** and set to 0 in two switch cases give `li rJ, 0` and then `addi rN, rJ, 0`. +Declared inside each case, they give two `li`. + +### `mr` against `addi rD, rS, 0` + +A late peephole changes `mr rD, rS` into `addi rD, rS, 0` only when the copy has an instruction before it in its basic block. +A `mr` that is first in its block stays `mr`. +The scheduler moves it after that, so its final position tells you nothing. + +An isolated `mr`/`addi` difference often comes from a dead instruction that took part in scheduling. +Case: `JKRAram.cpp` `firstSrcData` (93.3% to 100%). The forwarding inline returned `BOOL`, but its callee returned `bool`. +The dead conversion displaced the copy. +Compare a forwarding inline's return type with its callee's: `BOOL`/`bool`, `int`/enum, `u32`/`s32`. + +A redundant cast is not free either. +`((u16)x & 0xFFF0) + 0x10` emits a dead `rlwinm` that the scheduler sees before MWCC removes it, and that changes the registers. +Write the narrower mask alone. + +### A move between two locals never coalesces + +`coalescenodes()` (`InterferenceGraph.c`) coalesces a move between two vregs only when both are temps. +A move between two named locals, or between a local and a front-end temp, always stays. +A move into a real register (`return local;`) coalesces unless the two interfere. + +So a dead `fmr f1, f0` before the epilogue means **two** variables: an inner variable feeds an outer one, and the function returns the outer one. +Copy propagation keeps `outer = inner` only when the outer variable has more than one definition, which means a single exit that other arms also write. + +```cpp +f32 result; // outer: 3 definitions, one exit +if (...) { + f32 pan; // inner + if (...) pan = 0.5f; else if (...) pan = ...; else pan = ...; + result = pan; // survives -> fmr f1, f0 +} else if (...) { + result = unk4 & 1; +} else { + result = 0.5f; +} +return result; +``` + +A `static inline` helper with the same chain also gives 100%, because an inlined multi-return function also gives the caller a temp. +When a helper and a plain local both match, prefer the form that invents no symbol. + +### Reuse the destination as the carrier + +An inline with **one** `return` gives the caller its local, and that coalesces with the destination. +An inline with **several** returns (a ternary chain, a clamp) gives a new temp. +When the last of a series of inlines has several returns and the function ends with a dead `fmr f1, f0`, thread every step through the destination: + +```cpp +result = linearTransform(x, xStart, xEnd, 0.0f, y, true); +result = expf(result); +result = linearTransform(result, 1.0f, expf(y), yStart, yEnd, true); +``` + +Case: `JALCalc::getParamByExp` went from 98.2% to 100%. + +### `int` against `s32` for a float conversion + +`int` and `s32` (`signed long`) are both 32 bits, but MWCC treats them differently: + +```cpp +int iVar1 = (param_1 + 1.0f) * 50.0f; // fctiwz; stfd; lwz; then stfd; lwz again +s32 iVar1 = (param_1 + 1.0f) * 50.0f; // fctiwz; stfd; lwz only once +``` + +With `int`, MWCC makes a second stack temp and converts again for later uses (and the frame grows by 8). +Two `stfd`/`lwz` pairs from the same `f0` in the target mean `int`. One pair means `s32` or `long`. +Case: `MSHandle::MSACos` went from 91.2% to 100%. + +### Reference locals + +A reference local before member accesses can change register allocation: + +```cpp +unk0[i].mPos.set(...); // the base can be loaded each time +unk0[i].mVel.set(0.0f, 0.0f, 0.0f); + +Node& node = unk0[i]; // the base stays in a register +node.mPos.set(...); +node.mVel.set(0.0f, 0.0f, 0.0f); +``` + +Use a reference local only when: + +1. the target clearly loads the base pointer once and uses it again, +2. direct access gives redundant loads that the target does not have, or +3. the diff makes you confident that the original had one. + +In other cases, use direct member access or getter calls, and let CSE do the rest. +Reference aliases used as a general method often damage offsets and registers. + +### An inlined function loads `this` once + +When a simple version loads a pointer many times and the original loads it once, the original can have used an inline: + +```cpp +field->nonInlinedFuncCall1(); // `field` can be loaded twice: +field->nonInlinedFuncCall2(); // the compiler assumes that calls change memory + +field->inlineFuncThatCallsBoth(); // `field` loaded once, as `this` +``` + +This can look the same as a reference local. + +### Intermediate variables + +Intermediate variables are a strong lever on register pressure, load and store timing, and scheduling. +Start with the simplest form and change it step by step: + +1. Try a fully inline arithmetic form (no intermediate variables). +2. Try forms with temporaries. +3. Change the grouping and the order, also with compound assignments. + +```cpp +f32 c0 = (z1 - z0) * (x2 - x1) - (x1 - x0) * (z2 - z1); + +f32 dx = x1 - x0; +f32 dz = z1 - z0; +f32 c0 = dz * (x2 - x1) - dx * (z2 - z1); + +f32 c0 = (z1 - z0) * (x2 - x1); +c0 -= (x1 - x0) * (z2 - z1); +``` + +Try forms that a human would write first. + +A named cast local, `T* x = static_cast(search(...)); x->f();`, is a common style in this codebase. +It often fixes registers where a call on the cast does not. + +## The stack frame + +### Frame arithmetic + +``` +frame = round8( 8 + round8(locals) + 4 × saved GPRs + 8 × saved FPRs ) +``` + +The saved registers are at the top: `stmw rN, (frame - 4×(32-N))(r1)`. +A 4-byte gap between the highest local and the `stmw` offset is rounding, not an object. Do not search for a declaration to fill it. + +Because of the rounding, one 4-byte slot can change the frame by 8 or by 0. +Thus, never judge one lever alone. Count the slots in `variables.txt`, or use `mwcc_izer.py stack`. + +**Read the direction first.** +Compare local offsets from the **top** of the frame, not from `r1`. +If the saved registers and the conversion temp are at the same distance from the top in both builds, the difference is **below** all named storage: dead temp slots. + +- A frame that is too **small** (a deficit) usually means that the original had more inline expansions or deeper inline chains at the same instructions. It rarely means a missing local. +- A frame that is too **big** (a surplus) means fewer inline expansions. + +### Frame layout + +- `assign_locals_to_memory` places slots in size passes (1, 2, 4, ... bytes), each pass in list order. +- Earlier declaration means a higher address. The first memory local is just under the conversion-temp pool, which is just under the saved registers. +- The int-to-float conversion temp is 8 bytes, 8-byte aligned, above every named local. +- The function's own parse-time temps are at the top. Below them are the inline expansions in creation order. +- Each inline expansion's locals are in reverse declaration order. Use the ROM offsets to read the declaration order of an inlined helper's locals. +- Removing one temp lowers everything above it. The frame and the offset of an early temp move together, so the pair is a second constraint. + +### What reserves a slot + +MWCC reserves slots for objects that the final code never touches. +Each temp keeps its slot, also when the optimizer removes every instruction that used it. + +An inline expansion makes one temp for each of these: + +- the **receiver**, when it is an expression (`pArray[i]->getName()`). A call on the function's own `this` makes none. Two calls on the same object share one receiver temp. +- each **argument that is an expression**. A plain variable argument makes none. The temp has the size of the parameter type (a `u16` parameter temp is 2 bytes). +- the **return value**, when the function returns a value. A `void` inline makes none. +- each **named local in the inlined body**. + +Per site: `4 × (expression receiver) + sum(sizeof each expression argument) + 4 × (named locals) + 4 × (non-void return)`. +Count it before you guess a spelling. + +| Inlined call | Dead 4-byte slots | +|---|---| +| `mFoo.get()`, `mFoo` at **offset 0** | 0 | +| `mFoo.get()`, `mFoo` an embedded member at a non-zero offset | 2 | +| `mPtr->get()`, `mPtr` a pointer member | 3 | +| any void setter | 0 | +| free or namespace function that returns a global or a computed value | 1 | +| free function that returns `void` or a compile-time constant | 0 | +| `obj->get()` on a **local** pointer or object | 0 | +| static member function that computes from its argument | 0 | +| static member function that returns a global | 1 | + +The return temp survives only a **bare** discarded call: + +| Statement | Slots | +|---|---| +| `(void)f();`, `(void)(f());`, `(f()) ? (void)0 : (void)0;` | 1 | +| `(void)(x < f());`, `(x < f()) ? (void)0 : (void)0;` | 0 | +| `(void)0;` | 0 | + +So a release-stripped assert reserves a slot only if its condition is a bare call to such an inline. + +In a chain, an accessor reserves frame only when its result is the receiver of **another inlined call**. +The last link of a chain is free. +An accessor whose result goes into an out-of-line `bl` is free. +Raw field access through the same pointers costs nothing. + +| `changeBck` expression | Frame | +|---|---| +| guard only | 0x18 | +| `getActorKeeper()` | 0x20 | +| `...->getMActorAnmData()` | 0x28 | +| `...->getUnk2C()` | 0x38 | +| `...->getAnmPtr(index)` | 0x38 | +| the same chain as raw fields | 0x18 | + +A few more costs: + +- `getList()->getFirst()` costs 2 temps. `JSUList* list = &mList; list->getFirst()` costs 1. +- `T* x = f(); x->g();` inside an inline body costs 2 temps per site, `f()->g();` costs 1. +- `JGadget::TList::iterator::operator++(int)` costs 3 temps per site; `++it` costs none. When a list loop is short by 0xC, try `it++`. +- A ternary can reserve more stack than the same logic written with `if`. +- A struct local reserves its slot, also when it lives in registers. +- An array local in a block reserves its slot, also when every index is constant. +- A redundant cast on a pointer that already has that type moves every offset below it by 4. +- Each int-to-float conversion **site** reserves 8 bytes. A conversion of a constant folds and reserves nothing. +- A constant local that MWCC folds at every use gets no register, so its slot stays. An `int` used in both integer and float contexts can cost two. +- An **unused** local keeps its slot. A used local of 4 bytes or less gets a register and no slot. +- A sub-word named local (`u8`, sometimes `u16`) can go to memory at the top of the frame and cost 8 bytes. `u32`, `int` and `s32` get a register. +- IRO makes temps when it linearizes the AST (`IroLinearForm.c`) and deletes them later. Their slots stay at the bottom of the frame. + +### `T x = f();` costs one more object than `T x; x = f();` + +When `f` is inlined, `T x = f();` builds the return value in its own temp and copies it into `x`: two objects. +Declare-then-assign puts the return value straight in `x`: one object. +No instruction changes, only the frame. + +- Frame one object too big: look for an initialization to split. +- Frame one object too small: look for a call result that should have a name. + +Case: `TSpcInterp::update` was 8 bytes too big with an inlined 12-byte pointer to member. Declare-then-assign gave 100%. +Case: `TSpcBinary::init` was 8 bytes too small. `u32 hash = calcKey(...); symbol->mNameHash = hash;` gave 100%. + +### A local can remove a bound temp + +`CInline_SetupArgsExpression` binds an argument that is unsafe to repeat (a call always is). +A read of an unmodified local is safe, so MWCC substitutes it and makes no temp: + +```cpp +interp->push((int)interp->pop().typeof()); // binds a temp for the argument +u32 type = interp->pop().typeof(); // no temp +interp->push((int)type); +``` + +Case: `spcTypeof` went from 96.3% to 100%. +This helps only when the new local gets a register. +If it needs a slot, it moves every offset above it. The same change dropped `execadd` from 100% to 96.6%. + +A call in argument position reserves no slot. +A call split into its own statement that assigns a carrier variable adds 4 bytes per split. + +### Named locals inside proven helpers + +A plain function (namespace scope, or a member that takes everything as arguments) reserves **nothing** when inlined, unless its body puts a value in a named local. +Then it costs one slot at each expansion. + +Case: `JAISystemInterface::setSePortParameter` was 6 slots short. It expands the UNUSED `setPortParameter` six times. +`TOuterParam* outer = param_2->getOuterParam(); outer->setParam(...)` in the helper gave exactly 6. +Case: `JAIBasic::loadDVDFile` was 8 bytes short. `allocHeap` with `void* result; if (...) result = ...; else result = ...; return result;` closed it. + +This is not a fabricated pad: the local is used, and the map proves the helper. +An UNUSED map symbol just before the failing function is the strongest hint for the helper. + +The 15-statement gate and the caller's frame pull in opposite directions. +An initialized declaration is one statement and one caller temp. +When a callee sits at exactly 15 statements, a new local must be paid for with a statement that you remove. + +### Measuring a deficit + +Measure a deficit before you guess a fix. + +1. Count the missing slots: `mwcc_izer.py stack`, or diff `variables.txt` against the ROM offsets. +2. Calibrate with a probe. `(void)SomeNs::getGlobal();` reserves one slot and emits no instruction. Insert N of them, and find the N whose `stwu` is the target's. +3. Line up every `r1` access of both sides. The gaps show how many bytes are missing and where. +4. `volatile f32 d[2];` at a candidate point confirms a size and a position. + +Clear every pad and probe out of a function **before** you measure anything else in it. +A pad sets the frame, so the next candidate competes with it and measures worse than it is. +Case: with three dead locals in `MSBgm::stopBGM`, `JAISound::getTrack()` measured 99.88%; without them it measured 100%. + +Never commit a probe, a pad or a `volatile char trash[N]`. +See the rules on frame deficits in [AGENTS.md](../AGENTS.md#fakematches-important). + +### Legitimate levers for a frame difference + +Each of these is ordinary source that a person writes. +Use one only when something more than the frame supports it (an instruction change, a sibling function, a reference, the map). + +- A real inline accessor that the rest of the codebase uses (for example `SMSGetMarDirector()->getConsole()`), applied at every site. +- A named local for a chain result that is the receiver of the next call. +- A proven helper with a named local in its body. +- `it++` in place of `++it` on a JGadget list iterator. +- Declare-then-assign, or the reverse. +- Constant locals that the original evidently had (for example a division count). +- Default constructor arguments in place of written-out values. + +Case (`System/EventWatcher.cpp` `evManiCoinDown`): `gpMarDirector->getConsole()->startAppearStar()` gave a frame of 0x28. +The accessor gave 0x30 with the wrong layout. The accessor plus a `console` local gave 100%. +In the same file, `evSetNextStage` is worse with the accessor, so the original used the bare global there. + +A signed loop counter against an unsigned bound changes the unrolled shape of a small helper. +If that helper is inlined, the change moves the caller's frame too. +So a frame difference in a caller can be a signedness bug in a helper's loop. + +## Vectors and math + +### Use the `TVec3` inlines + +`JGeometry::TVec3` and the other `JGeometry` types have many inlines, and the original code used them most of the time. +When plain math does not match, try the inlines. + +### Multiplication by zero or one + +MWCC removes `1.0f * x` and `0.0f * x` that the source writes, but not when they come from an inlined function. +This is common when the code builds an "up" vector and uses `cross`. +Write `1.0f * unk170.z - 0.0f * unk170.y` as a real up vector and a `cross` call. + +### Constructor against component assignment + +```cpp +JGeometry::TVec3 pos(x, y, z); // all loads, then all stores + +JGeometry::TVec3 pos; // load/store pairs, one component at a time +pos.x = x; +pos.y = y; +pos.z = z; +``` + +`v.set(a, b, c)` computes all three arguments first (`z` first), then stores x, y, z. +Separate assignments store each component before the next is computed. +A store of `.x` before the code that computes `.y` means separate assignments. + +When the components share a factor in an object that the computation also writes (for example random draws on the emitter that owns the seed), separate assignments load the factor again each time. +One load means `set(getRandomSF(), getRandomSF(), getRandomSF())` and then `scale(unk204)`. + +### `zero()` against `set(0, 0, 0)` + +`.zero()` stores in **reverse** order (`z`, `y`, `x`). `.set(0, 0, 0)` stores in forward order. + +### `operator=` against `set()` + +```cpp +node.mPos = param_1; // lwz/stw (word copy) +node.mPos.set(param_1); // lfs/stfs (float copy) +node.mPos.set(ex, ey, ez); // lfs/stfs, like component assignment +``` + +The `TVec3` copy constructor is `TVec3(const TVec3& b) : Vec(b) { }`, and the ROM's weak copies are 0x1C of `lwz`/`stw`. +Any new `TVec3` copy theory must keep the weak `set`, `__ami__` and `TSplinePath::getPoint` out of line. + +### A `Vec` passed as `const TVec3&` + +A `Vec` expression bound to a `const JGeometry::TVec3&` parameter makes a temp through `TVec3(const Vec&) { set(b); }`. +If the conversion is inside an inline body, that `set(const Vec&)` stays a `bl set__Q29JGeometry8TVec3FRC3Vec`. +If it is written directly in the function, `set` expands. +So a single `bl` to that weak `set` means "a `Vec` went into a `TVec3` reference, one inline level down". +Each such conversion is also a 12-byte frame temp. + +### Fused multiply-add is per expression + +`a + b * c` in one expression gives `fmadds`. +The same product in its own statement gives `fmuls` and `fadds`. +Inlined `TVec3` operations (`sub`, `scale`, `add`) are separate statements. +So a ROM with no `fmadds` where the math is clearly `a + b*c` used vector operations or split statements. + +### `fnmsubs` + +Code like `B - A * C` or `A * C - B` compiles to `fmsubs` or `fnmsubs`. +`fnmsubs` is technically `-(A * C - B)`, but MWCC uses it for `B - A * C`. +Ghidra and m2c write `-(a * b - c * d)`. Rewrite it as `c * d - a * b`. + +### Zero components and `+=` + +When the target adds a literal `0.0` to some components (`lfs f4, @zero; fadds f4, f4, f0`), the original built a vector with literal zeros and then added another vector: + +```cpp +JGeometry::TVec3 point2(0.0f, -1.0f * unkC.y, 0.0f); +point2 += data.unk58; +``` + +### `-1.0f * x` against `-x` + +`-x` gives `fneg`. +A separate `fmuls` by the `-1.0` constant (not fused into a following `fmadds`) means that the source wrote `-1.0f * x`. +Case: `BathWaterManager.cpp` `calcBathtub`. + +### Literal order shows which constants came from inlines + +With `-inline deferred`, the literals inside an inline body are created at expansion time, after every literal of the caller's own text. +If the target creates a constant before a literal that the source clearly parses first, that constant is in the source text, not behind an inline. +A deferred inline call is also evaluated after the plain operand: `PI() * x` gives `fmuls fD, fx, fpi`, and `3.14159f * x` gives `fmuls fD, fpi, fx`. +Case: `TCylinder::makeDL` went from 98.4% to 99.5% with pi as a plain literal. + +### `MsMin`/`MsMax` and `.sdata` floats + +MWCC puts float literals in `.sdata2` (read-only). +A float constant in `.sdata` (writable) is a temp made for a `const T&` parameter. +A `.sdata` pair such as (1, -1) or (3, -3) is the clamp idiom: + +```cpp +if (v > 0.0f) + result = MsMin(v, hi); +else + result = MsMax(v, lo); +``` + +`MsMin`/`MsMax` (in `MarioUtil/MathUtil.hpp`) take `const T&`. +The return value is copied into the variable with one more `fmr` per arm, which a plain ternary does not do. +A TU with no `.sdata` section cannot use them with literal arguments. +`MsClamp` takes its floats by value (the map gives `MsClamp__Ffff`). + +## Loops + +### The array index type changes strength reduction + +`-opt nostrength` does **not** stop strength reduction in the back end. +`array[i]` in a loop normally becomes an induction variable (`addi rN, rN, sizeof(T)`) and `lwzx`. +`isinductionvariable()` accepts only `mulli`, a pure left shift, and indexed loads and stores. +A `clrlwi` from a `u16` index breaks it, and the `mulli` stays in the loop. + +Symptom: the target has `li rX, 0`, `addi rX, rX, ` and `lwzx`, and our build has `clrlwi` and `mulli`. +Fix: reach the element without a `u16` index. + +A signed counter against an unsigned bound (`int i < u32 size`) gives a shorter unrolled body than `u32 i`. + +### Induction variable copy (open problem) + +In some ROM loops, the derived induction variable starts as a copy of the counter (`addi r30, r29, 0` after `li r29, 0`). +Our build starts it with the constant (`li r30, 0`). +`initializeinductionvariable` emits the copy only for indexed loads and stores whose base or index is the counter. +First, check whether the loop is an inlined helper. Case: `JASystem::Dvd::updateBuffer` was the inlined `writeBufferSize`. + +### Pointer advances + +Two pointer advances usually merge into one `addi rX, rX, 2`. +`adjustforward()` (`Peephole.c`) moves an `addi rX, rX, imm` forward in its block, into the displacement of the next memory access through `rX`. +It stops at a store of `rX`, a branch, a call, or an indexed access through `rX`. +`*p++` twice, `*p = a; p++; *p = b; p++;`, and `p[0]/p[1]` plus `p += 2` all merge. + +To keep two separate `addi 1`, put a cast on the dereference: `status->stickX = *(s8*)data++;`. +The update forms (`lbzu`/`stbu`) come from a later peephole. + +### Pointer add operand order + +For an address, MWCC emits `add rD, rA, rB`, and `rA` shows the domain of the arithmetic: + +- Pointer arithmetic normalizes: `off + p`, `p + off`, `p[n]` all give `add rD, rBase, rOffset`. +- Integer arithmetic does not: `off + (uintptr_t)p` gives `add rD, rOffset, rBase`. +- A constant displacement with the index in `rA`: `*(u16*)&p[i * 4 + 6]`. +- `&p[c + i]` keeps `addi rD, rD, c` in the loop with the index first. `p + i + c` puts the base first, but MWCC hoists `p + c`. +- `p + i` with no constant, and `p += i`, keep the source order. + +When the target holds a pointer in `r0`, name the byte offset and add the base into the same variable: + +```cpp +uintptr_t entry = bank_id * sizeof(FabricatedWaveBankEntry); +entry = (uintptr_t)mWaveBankList + entry; +``` + +### Constant hoisting + +MWCC hoists constant loads (`lfs`, `lfd` from SDA) out of loops. +The set of hoisted constants depends on the expressions in the loop, the inlines it calls, and the order of operations. +If the target hoists a constant and our build does not, the loop body or an inline in it has a different structure. +Do not try to move the load. + +### Local arrays and unrolled loops + +MWCC can put the elements of a small local array (`f32 dist[3]`) or struct in registers and reorder them. +When the target keeps the array on the stack (consecutive `stfs`/`lfs` in a strict order), write the fill as a small `for` loop that MWCC unrolls: + +```cpp f32 dist[3]; for (int i = 0; i < 3; ++i) dist[i] = unk24[i]; ``` -Concrete case (`DrawUtil.cpp` `TSilhouette::loadAfter`, a Cramer's-rule solve -over `f32 atten[3]`/`dist[3]`/`m[3][2]`): element-by-element assignment needed -three `(void)&` hacks and still only reached ~92% (residual store scheduling); -rewriting the `dist` copy and the `m` fill as small unrolled `for` loops removed -the hacks and took it to 99.7%. Prefer the loop form — it's both cleaner and a -better structural match than address-of forcing. +`(void)&arr;` also works, but it is a fake artifact. Use it only to find the other problems first. +Case: `DrawUtil.cpp` `TSilhouette::loadAfter` went to 99.7% with no address-of hacks. -## Triviality of a type influences codegen +## Casts, types and signedness -A user-declared destructor (even an empty `~T() {}`) makes a class non-trivial, and MWCC pins non-trivial types to memory instead of promoting them into registers. -Triviality propagates: a non-trivial member/base makes the enclosing class non-trivial too. +### Narrowing casts are a hint -Two symptoms of a *missing* destructor, both meaning "the target keeps this in memory but our build cached it in a register": +A `clrlwi` or `extsh` before a use rarely means that the original had a cast. +Look first for a parameter, a return value, a local or a member type that narrows the value. -- **Struct fields reload across calls in the target, but ours caches them.** - A trivial struct is scalar-replaced (fields live in registers across calls); a non-trivial one is reloaded from memory after any opaque call. -- **The target spills a freshly-`new`'d object pointer to the stack and reloads it as `this`, but ours keeps it in a register.** - For `new T()` with a non-trivial `T`, the constructed object is a separate address-taken temporary with a stack home, so `this` is reloaded from the stack during construction. - Only visible when the constructor is inlined (out-of-line ctors call a real `bl` and never show it). +### Signedness and width show in the compare -Fix: give the smallest offending value/helper type an empty `~T() {}` and re-check. -This is a global change, so re-run the baseline — one destructor can fix (or shift) many callsites at once. -Concrete case: adding `~TMsRange()` (a field of `TSmallEnemyParams`) took several `TFooManager::load` functions from ~95% to 100%. +`cmpwi` against `cmplwi`, and `extsb`/`extsh` against `clrlwi`, give the sign and the width of a value. +Fix the declared type. Do not add casts. + +### Common store idioms + +`u8` to `f32` (for example from a `GXColor`): + +``` +lbz r0, OFF(rBase) +lis r4, 0x4330 +lfd f1, "@magic"@sda21(r0) ; 4503599627370496.0 +stw r0, X(r1) +stw r4, X-4(r1) +lfd f0, X-4(r1) +fsubs f0, f0, f1 +stfs f0, FIELD(rThis) +``` + +Source: `unk84.x = (f32)bodyColor[0].r;`. + +`(u8)` on an `int` return before a store to a `u16` field: + +``` +bl JUTNameTab::getIndex(...) +clrlwi r0, r3, 24 +sth r0, FIELD(rThis) +``` + +Source: `mField = (u8)getIndex(...);`. Without the cast, MWCC emits `sth r3` directly. + +### Casts of pointer variables are free + +`(const u8*)(const void*)param_1` and `(JKRHeap*)unk0` change nothing. +For a cast between a pointer and an integer, always use `uintptr_t`, never `u32`. On PowerPC this changes no byte. + +## Data, sections and symbols + +### Sections + +- An array with an explicit size of 8 bytes or less can go to `.sdata` (`@sda21`, `lwzx`). An **unsized** array (`fileNames[]`) always goes to `.data` (`lis`/`addi`). Case: `BathWaterManager.cpp` `fileNames[]`. +- A static member whose type is its own class (`H8 H8::probeSelf`) has no size when MWCC reads the declaration, so it goes to `.bss`, not `.sbss`. Use a nested class type. The mangled name does not change. +- A constructor does not stop small-data placement. +- An anonymous aggregate literal from a header inline compiled **in the precompiled header** goes to `.data`. The same literal in a `.cpp` goes to `.rodata`. When the target reads a constant from `.data` and ours reads it from `.rodata`, the original called the header inline. A high `@NNNN` number also shows a PCH object. +- The const-ness of a global changes its section. + +Section size check: `original section size = linked size in the map + sum of UNUSED sizes in that section`. +Compare it with `powerpc-eabi-objdump -h` of our object. +An extra `.rodata` literal moves every later offset (MWCC addresses `.rodata` as `base + immediate`). An extra dead `.data` object moves nothing. + +### Function-local statics: `init$NNN` is the guard + +A function-local `static` with a non-trivial initializer (for example `static JGeometry::TVec3 pos(1815.0f, 1500.0f, 1550.0f);`) makes two `.sbss` symbols: + +- the variable (`pos$NNN`), +- a 1-byte guard `init$NNN` (the name is always `init`). + +```asm +lbz r0, init$NNN@sda21 +extsb. r0, r0 +bne .L_skip_init +... initializer code ... +li r0, 1 +stb r0, init$NNN@sda21 +.L_skip_init: +``` + +Do not write the guard as a `static bool`. +It is the guard of another static with a real constructor. +Aggregate-initialized PODs and zero-initialized statics have no guard. + +Statics inside an inline function (also a member function defined in the class body) get `$localstatic$`. +`` is the developer's own variable name. + +### Static arrays of pointers to member functions + +MWCC does not emit such an array as data. +It emits one 12-byte constant `@NNN` per non-null element, and `__sinit` copies each one into the array with `lwz`/`stw`. +Null elements come from `__ptmf_null`. +The map's closure lists each `@NNN` with the function it points to. Pair them with the `__sinit` copies to recover the table. +Track the loads per destination register, not as a queue. + +### Mangling + +- A symbol in an anonymous namespace has an `@unnamed@` prefix. A local symbol without it is a plain `static`. +- A namespace-scope function mangles like a static member of a class with that name. `(func,local)` with a qualified name in the map is a `static` function in a namespace (a class member is never local), or a function template instance. Write it in a `namespace X { ... }` block. +- A local class of a member function mangles as `Q2$_cpp`. + +### The unique-ID counter + +MWCC names literals, labels, temps and local classes from one shared counter. +The gap between two `@NNN` numbers counts the entities that the source makes between their first uses. +This measures things that make no instruction. +Read the target numbers in `build/GMSJ01/asm/.s` and ours with `nm`, and align them by order, not by value. + +| Construct | IDs | +|---|---| +| an extra inline level on a call | 1 per site | +| a named pointer local in place of a repeated `p->` | 1 | +| `if (a) { if (b) ... }` in place of `if (a && b)` | 1 more | +| `if (c) ...;` | 2 | +| each extra `&&`/`\|\|` term | +1 | +| `if`/`else` | 3 | +| `while` | 4 | +| `for` | 4 at the head, +1 at the closing brace | +| an anonymous `enum` block in a class | 1 | +| plain statements, calls, float and string literals | 0 | + +A header inline costs 0 at the call site. +Use the counter to choose between spellings that give the same instructions, but several sources can give the same count. + +### Templates + +Every implicitly instantiated member of a class template is **weak**. +A TU that can see a member's body emits a weak copy. A TU that sees only the declaration emits an external reference. +An extra weak copy in an early TU takes the symbol away from the TU that the ROM got it from. +Put such bodies in a `
.tpp` file and include it only from the TUs that the map shows as owners. + +An explicit instantiation (`template class TViewObjPtrListT;`) makes the members **global** and sets their emission order. +MWCC 1.2.5 does not accept a qualified name in the directive: wrap it in the namespace. +Add these directives one at a time and measure each. +MWCC 1.2.5 has no `implicit_templates` pragma. + +### An `asm` function turns off the peephole + +A whole-function `asm` definition turns off both peephole passes for every function **after** it in the TU. +An `asm { }` block inside a C++ function does not. +Signature: a group of functions at the end of a TU with no record-form instructions (`rlwinm rX; cmpwi rX, 0` in place of `rlwinm.`) and no `lbzu`/`stbu`. + +## Recognizing source patterns from raw asm + +### Initializer-heavy functions + +For a function that is mostly field assignments (`init()`, constructors), read the raw `.s` file: + +1. Find the function in `build/GMSJ01/asm/.s` by its mangled name (`.fn `). +2. Map each `stb`/`sth`/`stw`/`stfs` to a field with the `/* 0xNN */` offsets in the header. Each store is one statement. +3. Float and integer constants are at the end of the same file as `.obj "@NNNN"` blocks, referenced as `lfs f0, "@NNNN"@sda21(r0)`. +4. String literals are in `.rodata`/`.sdata2`, referenced as `addi r4, r30, 0xNNN` from the TU's rodata base. +5. Compare the offsets one by one with the source. Anything in the asm but not in the source is missing. + +### `JSUMemoryInputStream` + +Game code mostly uses the `operator>>` overloads, sometimes chained (`stream >> vec.x >> vec.y >> vec.z;`). +The frame of the `JDrama::TNameRef::load` overloads matches only with these forms. +`stream.readU16()` also occurs. Choose by codegen at each site. +A raw `stream.read(&someInt, 4);` is the least likely form. + +## Known accessor verdicts + +These results come from sweeps over the whole game with `ninja baseline` and `ninja changes_all`. +Do not repeat a refuted sweep without new evidence. + +| Accessor | Verdict | +|---|---| +| `TFlagManager::getInstance()`, `SMSGetMarDirector()`, `TMarDirector::getConsole()` | Real. Use them outside the owning classes. A few sites read the raw global because the ROM shows it. | +| `SMSGetMarDirector()->getGamePad(N)` | Real (proven by `TCardSave::load`). | +| `SMSGetMarioParticleManager()` | Refuted: 77 functions worse, 1 better. Use raw `gpMarioParticleManager`. | +| `SMSGetMap()` | Refuted: 51 worse, 7 better. Use raw `gpMap`. | +| `TPlacement::getPosition()` | Fake. Thousands of matching functions read `mPosition` raw. Do not use it to close a frame deficit. | +| `TLiveActor::getVelocity()`, `getModel()` | Mixed use is real. Choose the getter or the raw field at each site by codegen, if no other function gets worse. | +| `TTakeActor::getHolder()` | Use it only where a protected `mHolder` would not compile: reads through another object. Own reads stay raw `mHolder`. | +| `J3DModelData::getVtxPosArray()` | Real. | +| `TParamVec::set` | Refuted at all 14 sites. | +| `JUtility::TColor` destructor | Refuted (see above). | +| a `JAIHeapBlock` accessor set from TWW | Refuted: SMS reads those fields raw. | + +The "protected field" test: imagine that a field is `protected`. +Use its getter only where the compiler would refuse the raw access: reads through another object, `self->` in nerves, globals like `gpMarioOriginal`. +A write through another object would not compile either: that is a sign of a missing setter-like inline.