Reproduce game weak data with a PCH - #130
Conversation
Report for GMSJ01 (9b62e09 - d83decd)📈 Matched code: 37.73% (+0.57%, +20624 bytes) ✅ 46 new matches
...and 16 more new matches 📈 42 improvements in unmatched items
...and 12 more improvements in unmatched items 📉 4 regressions in unmatched items
|
d8cac9d to
6a41574
Compare
KakarottoCake
changed the title
2002949 to
a927788
Compare
533a056 to
f8aa717
Compare
|
Follow-up commit a7e5756 centralizes the common game compiler profile in cflags_game: -opt all,nostrength, -inline deferred, and -prefix SMS.mch. The PCH build and the three evidence-backed no-PCH object exceptions retain explicit flag lists; THPPlayer remains on its separate profile. Verified with a full build (mario.dol: OK), ninja baseline/changes_all (no regressions), Python syntax validation, and git diff --check. |
6854611 to
9b9140b
Compare
|
Edited: this comment predated the rebase. The commit is now Resolves the "Known edge case" that was in the description — Root cause. Retail's MWCC picks the owning TU by key function: the first non-inline virtual in declaration order. Ours was Second fix.
Codebase sweep. I checked for the same bug elsewhere by resolving every // NOTE: has to be defined not first to get the vtable
// to emit to the correct TU
virtual ~JKRArchive();My sweep had been reading the pre-sweep header on this branch. No follow-up PR is coming, and none is needed. |
eb2b73a to
ca3368d
Compare
KakarottoCake
changed the title
|
Rebased onto current Three things changed since the last look: CI. The Description. The Results table was measured against a superseded baseline and the "Known edge case" section described the MapObjInit stand-ins, which Re-audited the PCH selection.
Per-unit across the 40: 23 improve, 17 no effect, 0 regress. Across the 696 non-PCH units: none would gain, and exactly three regress under the global build — The 17 inert units are kept deliberately: seven are still at 0% code and 0% data, so there is nothing for the PCH to act on yet, and two are already at 100% data. Happy to trim them to 23 if you would rather the list carry only units with present-day evidence — it makes no difference to the build either way. |
Retail puts __vt__14TMapObjGeneral, the getFlushTime weak body and the @32@ adjustor thunk in MapObjInit.o. We were emitting all three in MapObjGeneral.o, which is why MapObjInit.cpp still carried hand-written stand-in literals to make up the missing data. MWCC picks the owning TU by key function: the first non-inline virtual in declaration order. Ours was loadAfter (MapObjGeneral.cpp); retail's must have been initMapObj (MapObjInit.cpp). initMapObj is an override of a TMapObjBase virtual, so moving its declaration to the front of the list cannot change the vtable layout -- and it doesn't, the table is still 0x1dc. All three symbols move to MapObjInit.o together. getFlushTime also had an empty body, which is 4 bytes; retail is 8 and disassembles to `lwz r3, mNormalFlushTime@sda21; blr`, so give it the real body. That mirrors getLivingTime directly above it, matches 100%, and silences the "return value expected" warning. With the data now coming from the real vtable, dummy/dummy2/dummy3/dummy4 are no longer needed and are removed -- no fakematch left in this file. matched data 50.0071% -> 53.5928% matched functions 7921 -> 7923 MapObjInit data 0.389% -> 44.203% mario.dol: OK; check-changed-symbol-order.py passes on both units. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ca3368d to
aecc701
Compare
aecc701 removed the hand-written stand-ins from MapObjInit.cpp -- the two Vec{1,1,1} and the u32[4]{0,2,1,3} -- but left the unit as a plain Object, so nothing supplied those bytes afterwards. Its .data went from 4 bytes short to 44, with every shared symbol shifted by a uniform +40. Marking it PCHObject restores them from the real header, which is what the rest of this PR does everywhere else. .data now matches and the unit is back to 4 bytes short, as it was before this branch. Two neighbouring units show a similar offset but are not this problem and are deliberately left alone: - GC2D/SelectMenu is already a PCHObject and receives the header correctly; both objects carry the same 40-byte triplet at the head of .data. Retail's extra 40 bytes there are scNormalStageTable at 0x190, which we do not emit yet. That is a source gap, not a PCH one. - GC2D/ConsoleStr has no triplet in the retail object at all, so it was never a PCH unit. Its -40 comes from scEtcShineConvTable, which we emit and retail does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All seven preprocessor macros in this file are gone, converted to static inline functions or folded into their single call site. Four of them had exactly one use, and one of those -- UPDATE_LIFE_SEGMENT_COUNT -- contained another one-use macro, UPDATE_LIFE_METER_COLORS. A 27-line macro used once is a function that was afraid to be a function. SET_THREE_DIGITS turned out to duplicate a helper that already existed, so it was removed rather than converted. No function changes match state: still 32/60 in the unit, data still 40 bytes (the PCH footprint from doldecomp#130), linkage still matches the map. perform() moves from 17.2% to 19.5% as a side effect -- the macros were being pasted into their callers, so removing them changes which bodies the inliner is offered and at what statement budget. Some conversions cost a little percentage and later ones gave it back; none cost a byte-identical function. Reported by troy and theAzack9 on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* GC2D/GCConsole2: decompile the HUD update and draw paths Implements the fifteen per-frame process methods, drawWater, drawJuice, drawWaterBack, loadAfter and startAppearBalloon, which were previously stubbed empty in the header, plus the Dolpic news tables and the pane helper routines they share. Byte-identical functions go from 14/41 to 32/60; the compared set grows because the formerly empty methods now emit. The remaining data gap is 40 bytes in .data, which is the precompiled-header footprint addressed by #130 and deliberately untouched here. Field mapping: several opaque char[] blobs are replaced with typed fields at identical offsets, evidenced from the target disassembly (unk70/72/73/74/76/77, unk2B8, unk424, and unk154 as a TBoundPane*[3]). Also fixes an existing typo in the second star loop, which assigned to unk134 rather than unk154. Three constructs here are fitted to the compiler and worth calling out rather than leaving for a reviewer to find: - The process methods are reached through one accepted helper layer below perform() so that MWCC offers them at a pass with a statement budget and refuses to expand them, emitting each with the weak linkage the map records. Called directly from perform() they are offered at the unlimited first pass and absorbed regardless of size. The helpers are grouped by HUD subsystem to stay readable rather than being opaque forwarding wrappers. - Four pane state helpers take their pane parameter as a reference to a pointer. The reference is never rebound; it is there because it produces the member reload sequence retail emits. Each was tested with a plain pointer and each regressed: updateDownPaneState -2, updateUpPaneState -4, updateUpPaneStateAfter -1, updateUpBlendPaneState -1. - setBlendDigit and the placement of updateCounterState() after the water-gauge interpolation are positioned so that TVec3<f32>::set falls past the inliner's statement budget and is emitted out of line, as the map requires, in the same five call sites retail has. The rogue include of M3DUtil/InfectiousStrings.hpp follows the existing placement in src/Enemy/graph.cpp and restores the MActorMtxCalcType_* table and the dummy string pair, closing .rodata entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * GC2D/GCConsole2: fold three single-use helpers back into perform `perform` was 4032 bytes short of retail. The gap was not missing logic but logic factored out of it: setCoinCounterDigits, setJetCounterDigits and getBalloonTextColor existed as separate functions in our object and not in retail's, which has that code inline. Each sat two call layers below `perform`, so the inliner offered it at a pass with a statement budget of 10 and refused it -- 48, 27 and 35 statements respectively. Folding each into its only caller puts the combined helper back at the unlimited first pass, where it is absorbed. All three had exactly one call site, so this moves code rather than duplicating it. `perform` goes from 13.4% to 17.2%, and 0x36ec to 0x3e28 against retail's 0x46ac. startLifeMeterDisappear was tried the same way and reverted. It has four call sites, so folding it duplicated an eleven-line block four times for 1.9 percentage points on a function that stays unmatched either way. Not worth the source. No other function changes state; still 32/60 in the unit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * GC2D/GCConsole2: replace the macros with ordinary functions All seven preprocessor macros in this file are gone, converted to static inline functions or folded into their single call site. Four of them had exactly one use, and one of those -- UPDATE_LIFE_SEGMENT_COUNT -- contained another one-use macro, UPDATE_LIFE_METER_COLORS. A 27-line macro used once is a function that was afraid to be a function. SET_THREE_DIGITS turned out to duplicate a helper that already existed, so it was removed rather than converted. No function changes match state: still 32/60 in the unit, data still 40 bytes (the PCH footprint from #130), linkage still matches the map. perform() moves from 17.2% to 19.5% as a side effect -- the macros were being pasted into their callers, so removing them changes which bodies the inliner is offered and at what statement budget. Some conversions cost a little percentage and later ones gave it back; none cost a byte-identical function. Reported by troy and theAzack9 on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * GC2D/GCConsole2: mark the fabricated inlines None of this file's helpers existed in the original source -- the retail build wrote that code inline at each call site, and they are here to keep the decompilation readable. Marking them says so, matching the convention used about 335 times elsewhere in the tree. 72 comments added, covering every fabricated helper including the ones that came out of the macro conversion in the previous commit. The group note above the pane-offset pair is replaced by per-function markers. The static data tables are real and stay unmarked. Comments only: the unit still builds byte-for-byte the same, 32/60, perform() 19.5%. Reported by troy and theAzack9 on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * GC2D/GCConsole2: remove fake layout helpers * GC2D/GCConsole2: fold fabricated drawing helpers * GC2D/GCConsole2: tidy external declarations --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
This reproduces the MWCC precompiled-header behavior responsible for the J3DJoint weak Vec literals appearing in
.datain retail game objects.extra_cflags=["-prefix SMS.mch"]-opt all,nostrengthand-inline deferredcentralized incflags_gameTMapObjGeneralvtable ownership, which removes the last stand-ins inMapObjInit.cppTMBindShadowBodyTWW references:
Results
Measured against
mainat the current rebase point, by toggling only the PCH:So the PCH is worth +0.57pp code, +4.73pp data, +27 functions.
build/GMSJ01/mario.dol: OK.Largest per-unit contributions:
NPC/NpcInitData+10280 data,Player/MarioInit+10016 code,MoveBG/Item+5088 data,Enemy/hinokuri2+3000 code / +1976 data,Enemy/gatekeeper+2584 data,Strategic/liveinterp+2140 code,MarioUtil/MathUtil+2096 data.Per-unit A/B across the 40: 23 improve, 17 no measurable effect, 0 regress. The 17 inert units are kept because they are premature rather than wrong — seven of them (
bombhei,bosswanwan,feetinv,killer,tinkoopa,ModelGate,MarNameRefGen_BossEnemy) are still at 0% code and 0% data, so there is nothing yet for the PCH to act on, and two (MapEventDolpic,MapEventSink) are already at 100% data.Selective PCH audit
The selection was re-verified on the current base with a three-way controlled experiment, changing only whether the PCH is applied:
cflags_gameGlobal is worse than selective (code 37.6024%, data 54.7896%, 8088 functions), which is what justifies keeping the list narrow rather than turning the PCH on project-wide.
Across the 696 units that do not receive the PCH, none would gain from it, and exactly three regress under the global build:
MarioUtil/RumbleDataStrategic/spcinterpMarioUtil/gd-reinit-gxThese are the same three no-PCH cases identified when the list was first assembled, so the boundary is unchanged.
No padding or source fakematch is used for any of these results.
MapObjGeneral vtable ownership
Retail's
MapObjInit.oowns__vt__14TMapObjGeneral, thegetFlushTimeweak body, and the@32@__dt__14TMapObjGeneraladjustor thunk. We were emitting all three inMapObjGeneral.o, and that missing.datais what the hand-written stand-ins inMapObjInit.cppwere compensating for.MWCC selects the owning TU by key function: the first non-inline virtual in declaration order. Ours was
loadAfter(MapObjGeneral.cpp); retail's must have beeninitMapObj(MapObjInit.cpp). BecauseinitMapObjoverrides aTMapObjBasevirtual, its slot is fixed by the base, so promoting its declaration cannot change the vtable layout — and does not, the table is still exactly0x1dc. All three symbols relocate together.getFlushTimealso had an empty body{ }(4 bytes, and the source of areturn value expectedwarning). Retail is 8 bytes and disassembles tolwz r3, mNormalFlushTime@sda21; blr; that static already existed, so the real body mirrorsgetLivingTime()above it and matches 100%.With real vtable data present,
dummy,dummy2,dummy3anddummy4are removed.MapObjInitdata goes 0.389% → 44.203%; the remainder is genuine unreconstructed content, not padding.A sweep of the rest of the codebase for the same bug — resolving every
__vt__owner in mario.MAP against the TU defining that class's first non-inline virtual — found no other mismatches (624 classes resolved of 1508 vtables).Validation
-prefix SMS.mchninjabuild and retail DOL checksumtools/check-changed-symbol-order.pypasses on all 10 changed.cppfiles