Conversation
Numeric literals instead of char literals, drop the redundant (int)(uint) widening on the byte height, and reuse the already-loaded LogicLayer word instead of reloading it twice. The (char) casts stay: negativeHeight is a byte and the original compares it signed. Measured with try_styles.py: baseline, clean and logicflags all 26.40%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
Drop two Ghidra artifacts: the (_score + (_score >> 0x1f & 3)) >> 2 idiom is the compiler''s signed divide by 4, and the distance locals were typed dword while every use cast them back to int. Measured with try_styles.py: baseline 34.30%, div4 / signed_score / signed_all all 34.40%; kept signed_all as the one with no casts left. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
The decompiler reports MSVC''s biased shift for a signed division by a power of two, "(x + (x >> 0x1f & 7U)) >> 3", literally. Rewriting the 22 sites in this namespace back to "x / 8" is what the original source said, and it matches considerably better: unitClimbing 32.5 -> 55.3 findEligibleUnitByTimeAndLocation 63.0 -> 77.8 ifOnADefensiveStructureSetDestination... 50.5 -> 64.9 checkTargetBuildingPossibilityOrState 26.5 -> 39.2 prepareProjectileTarget 42.5 -> 49.0 acquireShootTarget 39.3 -> 41.0 spawnUnit 46.8 -> 47.6 findNearestEnemyAndHeadTowardsIt 34.3 -> 34.4 Namespace average 74.32% -> 74.70%, no function regressed. The one site in processEntityDamageToUnit that measured 0.1% worse keeps the shift, with a remark saying why. Adds undiv.py (a balanced-paren rewriter; a regex cannot capture the dividend) and test_undiv.py covering the near-misses that must be left alone: a shift that disagrees with the mask, a mask that is not 2^k - 1, and a bias taken from another variable. Also fixes two ways the batch tools quietly report on the wrong thing: try_styles.py narrowed cmake/openshc-sources.txt.local to one function and never restored it, and reccmp_report.py read a stale diff.json without saying that it covered almost none of the build list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
The compiler cannot mask to take "x % 16" of a signed int, because the
result keeps x''s sign, so it masks the low bits together with the sign bit
and repairs the negative case afterwards. The decompiler reports both halves:
v = x & 0x8000000f;
if ((int)v < 0) {
v = (v - 1 | 0xfffffff0) + 1;
}
That is "v = x % 16". Five sites, four of them in updateUnits:
updateUnits 35.1 -> 37.8
getUnitStateTextParameterAndResourceType 64.8 -> 64.9
The repair exists only because the operand is signed -- an unsigned
remainder is a bare "and" -- so the dividend has to stay signed in our
source as well. fixedRng and the unit slot ID are uint fields and the
decompiler''s "+ 4U" makes its sum unsigned too, so unmod.py drops the U
suffixes and casts to int; without that the match would drop rather than
rise.
Namespace average 74.70% -> 74.72%, nothing regressed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
The health bar was carrying the decompiler''s mangled magic-multiply for a
division by 10:
(x / 10 + (x >> 0xf)) - (short)((longlong)(int)x * 0x66666667 >> 0x3f)
Both correction terms are "-1 when x is negative, 0 otherwise", so they
cancel exactly and the whole expression is x / 10 -- a health percentage
scaled to a ten-segment bar, which is what the field is called.
The decompiler had also reused _entityShootingUnitID to hold that
percentage; the value now goes straight into the field it belongs to.
28.7% -> 31.4%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
…36.4%
All three are the same mistake: the decompiler''s gotos jumped into blocks
with several predecessors, and removing the gotos dropped the block instead
of duplicating it.
1. Flying cow damage. The original falls through to a block it shares with
the other siege projectiles:
uVar5 = -(uint)(_entityType_2 != UT_S_SHIELD) & 0x1c2;
LAB_00531f5d:
_damage = uVar5 + 0x32;
We kept the first line and lost the second, so a cow assigned a local
nothing read and left _damage holding the entity height from the altitude
check above. It is now 50 against a shield and 500 against anything else.
2. Hit sounds 2 to 7 were unreachable. The break sat outside its if:
if (_hitSoundVariant == 1)
sfxOffsetInArray = FX_BODY_HIT2;
break; // always taken
if (_hitSoundVariant == 2) { ... } // dead
so every non-siege unit played the same hit sound.
3. The UT_S_SHIELD case set no sound at all and fell out of the switch,
leaving sfxOffsetInArray uninitialised on that path. In the original that
case shares LAB_0053231d with hit sound 1, which sets FX_BODY_HIT2.
Also clears the 25 decompiler-generated names here: bVar4 is the flag that
picks FX_GIRL_DIE, so it is _usesFemaleDeathScream; uVar5 was serving three
unrelated purposes and is split; sVar3 and the psVar1 field pointer are gone
in favour of the fields themselves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
All 62 decompiler-generated names are gone. sVar9 was one register the
decompiler had merged across seven unrelated values -- a manned engine
target, a saved destination x, a pitch ditch index, a building index through
the same field, an enemy tribe id, the chosen target and an attack tile y --
so it is split rather than renamed. Three sites share the identical line
"shootTargetedUnit = sVar9;", so this was done by attributing each read to
the assignment that most recently preceded it, not by matching text.
iVar7 was merged across three values, one of which hid a trap: the old value
is still an argument to the call that overwrites it.
&& (iVar7 = arrowShootingRelated(..., iVar7, ...), 0 < iVar7)
The call is now lifted out of the condition, so the enemy y it reads and the
flight distance it returns are separate variables.
Measured with try_styles.py: baseline and named both 41.00%.
Adds unptr.py and test_unptr.py for the field-walking pointers, and closes
two ways the batch tools reported on a stale build: build_quiet.py now exits
non-zero on BUILD_FAIL so "build && report" stops, and load_diff() warns when
a source file is newer than diff.json. build.bat exits 0 even when
compilation fails, so without these reccmp silently reports the previous
DLL''s numbers -- which it did here, and the unchanged figure was mistaken
for a neutral change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
…39.0%
"field += n" on a field the compiler already holds an address for comes back
from the decompiler as a pointer and a store through it:
piVar1 = &this->units[i].attackedBy;
*piVar1 = *piVar1 + 1;
MSVC regenerates the same instructions from the field form, and the project
style asks for named fields rather than pointers walking over structs.
unptr.py rewrites only the shape where the store immediately follows the
pointer, which matters for more than simplicity: a pointer freezes the
address while the field form re-evaluates the index. With nothing between the
two lines the index cannot have changed. Where the decompiler sets a pointer
up and dereferences it further down -- past a call, or past a write to the
index -- the two forms can genuinely differ, so piVar1 and psVar4 are still
declared for those.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
…> 40.9% The decompiler reloads uVar9 from DAT_CurrentUnitSlotID::instance eighteen times, every one of them just after a call. A source-level local would give one load; a reload at each point where a call could have clobbered the register means the source read the global directly and the compiler cached it in between. Reading it directly is worth 1.9%, and normalized 38.0% -> 41.5%. uVar9 also held two unrelated values -- a target x, and the facing direction taken mod 8 in case 0x16 -- and bVar13 held three unrelated bytes: a height, a fade type, and the alpha it maps to. Those are split. No decompiler- generated names are left in this function. The nine remaining field-walking pointers stay, and they are worth a remark. All nine are the set-up-then-use-later shape unptr.py skips, and all nine were checked by hand as safe to remove: no index can change in between, and the one with a call between the setup and the store takes its address from a local and a fixed global base. Removing them anyway measured 2.7% worse (38.4% against 41.1%), so where the dereference is separated from its setup the original really did hold a pointer. unptr.py''s adjacent-lines-only rule was chosen for safety but turns out to be the right line for matching too: the adjacent shape gained 1.2% over 60 sites, this one loses. They are named _intFieldPtr and _shortFieldPtr rather than deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
…nditions The decompiler writes a field read inside a condition as an assignment joined by a comma operator, reusing one register for unrelated fields while it does: _buildingID held both a unit type and a state inside the same condition. Nesting the tests gives each field its own name and keeps the short-circuit order. Measured tied at 41.10%, so kept for readability. The loop head now uses the loop''s own index instead of re-reading DAT_CurrentUnitSlotID::instance, worth 0.2%. This refines the previous commit rather than contradicting it: the choice is per-region, not global. After a call the original reloads the global, so read it directly there; in a call-free stretch like the loop head it keeps a scaled index in a register (imul eax, eax, 0x490) and reuses it, so use the local. No call runs between _currentUnitID being assigned and the end of that stretch. Two hypotheses from reading the diff measured worse and were dropped: swapping the arms of the lord/disappear if (40.8%), and an apparent field mix-up that turned out not to exist. The diff showed us writing destinationNeeded where the original writes updateTickTracker, but counting the writes on each side gives three and one on both, so difflib had simply paired two unrelated adjacent instructions that differ by three characters. A substitution pair in the alignment is not evidence that two instructions correspond; count occurrences per side before believing one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
…ions measure() parsed the percentage out of reccmp_report.py''s printed output, which is formatted to one decimal. Every variant sharing a first decimal therefore compared as exactly equal, and the tie check confirmed the tie. It now reads the ratio from diff.json and prints four decimals. This was not hypothetical. A processMeleeInitiation variant that rewrote the occupancy-row byte arithmetic as directionTranslationMatrix indexing printed the same "29.20%" as the baseline and was kept for readability; it was really 29.1840% against 29.2042%, one instruction worse. Reverted. Large results are unaffected, because those were verified with "reccmp_report.py cmp" against a snapshot, which compares the floats. The one change that survived measurement here is uint -> int for _neighbourHeights, an exact tie at 29.2042% and the more honest type: those values are only ever used in signed subtraction. processMeleeInitiation does not move on source shape. Nine hypotheses now: caching the flags global (24.2%), looping the eight unrolled height calls (21.6%), both (16.5%) and hoisting a shared row pointer (21.4%) are all much worse; array declaration order and the element type are exact ties. The diff shows the original holding zero in bx to compare against, addressing locals through ebp at a negative offset, and running a 4-byte smaller frame -- all register allocation and frame layout, which the source does not reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
gynt
force-pushed
the
reimpl/UnitsState
branch
from
September 28, 2026 21:22
13b2b38 to
0ea2c2b
Compare
…ad per field diff_types.py reports movsx on the original against movzx on ours for OrganismLayer and for units[].owner. Both are declared short and UnitTypeShort is a typedef short, so the generated headers are right -- the fault is at the use site. Reading the same field several times in mixed contexts makes MSVC emit movzx plus a separate "movsx r, r16"; the original loads it once, signed. One int local per field gives that single movsx: organism_local 25.55% -> 25.88% OrganismLayer, read three times owner_local 25.55% -> 28.63% units[].owner, read three times both 25.55% -> 30.26% more than additive Confirmed at 26.4% -> 31.1% with the struct resolvers enabled, and diff_types.py now reports no type hints for this function. This settles an ambiguity in AGENTS.md, which says to access fields repeatedly rather than copy them into locals, and separately that naming a repeatedly read field is unpredictable and must be measured. Both hold; the EXTEND hint is the missing discriminator. When the diff shows movzx plus movsx against a single movsx, the local is the original''s own reading rather than a gamble. processEntityDamageToUnit also stops reusing one local for both a shooter id and a tile -- the decompiler could reuse that register because the cow-poison branch returns before the id is needed again. An exact tie at 36.4%. Declaring its three int flags as bool, which is what the decompiler shows, measured 0.43% worse and was dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
diff_types.py flagged movsx on the original against movzx on ours for
units[].unknownTestAgainst0_2. The field is already short and so is the local
that reads it, which makes the load 16-bit; the original sign-extends into a
32-bit register, which is what an int local does. Widening that one local
takes the function from 85.7% to 95.2%, and normalized to 100% -- only call
targets differ now.
Widening an existing local is not the same operation as introducing one, and
the difference decides whether this family of fixes helps:
* introducing a local changes how many times memory is read, and MSVC often
prefers to compare against the memory operand instead. Doing that to a
field feeding an == constant chain cost 11.8% in playHurtSFXForUnit.
* widening a local only changes the extension width of a read that already
happens once, so it cannot add a load.
setMoveDelayForUnitsOnSameTiles has the same defect at array scope: its
scratch array is ushort and all fifteen reads cast back with (short). The
original reads the element signed in one movsx. Declaring the array short says
that without the casts and measured an exact tie at 19.3211%, so it is kept
for the fifteen casts it removes rather than for the match.
Also completes the earlier try_styles.py fix. Restoring the build list was not
enough: the trials leave the DLL built from the narrowed list, so diff.json
kept reporting one function at the last variant''s score and reccmp_report.py
--run could not repair it. It now rebuilds with the full list before exiting,
which costs a few minutes per invocation and is worth it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
Same fix as resetUnitMovementState, applied to every short/ushort local that
holds a 16-bit field read in the functions diff_types.py flags. The local is
declared after the field, so the load is 16-bit and needs a second instruction
to sign-extend; the original sign-extends in one movsx, which is what an int
local does.
setUnitFacingDirectionForTargetXandY 86.1% -> 90.4%
acquireShootTarget 41.0% -> 43.3%
setUnitFacingDirectionBasedOnBuilding 71.4% -> 73.5%
processEntityDamageToUnit 36.4% -> 38.0%
processUnitMove 61.4% -> 62.5%
updateUnits'' _aiBehaviour was widened too and measured 0.3% worse, so it
stays short. That fits the boundary this family of fixes obeys, now confirmed
three times independently:
helps the value feeds arithmetic, indexing, or a comparison against
another variable
hurts the value feeds comparisons against constants, which MSVC compiles
against the memory operand directly
_aiBehaviour feeds a switch and an == 10 test; playHurtSFXForUnit''s unitType
feeds a twelve-way == chain and cost 11.8% when given a local at all.
_pathIndex went from ushort to int, which zero-extends either way, so there
was no movsx to gain there -- processUnitMove''s gain came from its
_destinationX and _destinationY siblings. Signedness is what matters, not
width.
All 18 candidates were changed in one batch and measured in a single build,
since each is confined to one function and reccmp reports per function; the
one loser was then reverted on its own.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
diff_triage.py''s BYTE column counts byte-sized [esp] accesses per side. Where it is nonzero only on ours, those locals are ints in the original. The direction was already evidenced from the other side: declaring three int flags as bool in processEntityDamageToUnit, which is what the decompiler shows, cost 0.43%. updateUnitFadeAndVisibilityNearStructures 39.4% -> 51.4% 16 byte accesses findNearestEnemyAndHeadTowardsIt 34.4% -> 37.2% 7 stopUnitIfNextToTarget 30.0% -> 30.8% 4 giveMoveCommand''s _canReachDestination was widened too and measured 3.0% worse, so it stays bool. It differs from the others in a way worth recording: the three that gained start from a constant and are assigned true or false, while this one is initialised from a BOOLEnum-returning call. `bool x = call()` emits a test that normalises the result to 0 or 1; `int x = call()` stores it raw, so there the bool conversion is part of the codegen rather than an artefact of it. So: widen a byte-sized local when it holds a flag set from constants, leave it when it is initialised from a call''s return value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
…al match _unitX held units[].x, a signed short, in a uint, so every later comparison against the int parameter x was done unsigned. int is the honest type. An exact tie at 36.1486%, as was widening the two parameter copies alongside it. This was an attempt at the FRAME mismatch diff_triage reports for this function (frame 8 on the original against 0x14 on ours, three stack slots only we have) and it did not shrink the frame. Recording the negative: unlike BYTE and the local width and signedness family, FRAME does not appear to be mechanically actionable. The namespace has exactly one instance of an unsigned local holding a signed field, so there is no batch here either -- the signedness defects came in clusters of width (18 short locals, 9 byte flags) while this direction is essentially absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
reorder_search.py hill-climbs the match by swapping statements that are provably independent of each other. MSVC decides register allocation partly from the order in which values become live, so the order of independent writes before a call changes which value is in which register at the call boundary. getFirstSelectedUnitID 30.8% -> 85.7% normalized 100% queueDisbandAndAttackCommand5Params 21.4% -> 50.0% three successive swaps updateMicroPosition 55.8% -> 64.6% All three are small functions that write several independent values before a call, which is exactly the boundary case the tool documents as its exception. On a small function the proportional effect is far larger than the "fraction of a point" the docs lead you to expect: getFirstSelectedUnitID''s swap is two writes to different fields, neither reading the other''s target, and it moved the function 55 points. queueDisbandAndAttackCommand5Params needed three swaps that each built on the last, 21.4 -> 35.7 -> 42.9 -> 50.0. A hill-climb finds that; reading the diff does not. Five of the eight functions swept found nothing, and the three that did were all in this shape. getEnemyUnitIDNearby, checkAnySelectedUnitCannotClimb, setupUnitSharingTileIDs and playHurtSFXForUnit have zero candidate swaps at all: the independence test excludes any statement containing a call, and dense control flow leaves it nothing to move. Namespace 74.99% -> 75.48%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
build.bat gives each worktree its own _MSPDBSRV_ENDPOINT_ and kills its own mspdbsrv.exe before building. build_quiet.py --keep-going calls cmakew directly and syntax_check.py calls cl.exe directly, so neither inherited that, and a server already wedged before the build started survived build.bat kill and still failed with C1090. common.build_env derives the same endpoint name; common.kill_pdb_server stops only this worktree instance; build_quiet.py retries the build once after a C1090/C1033. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NaSkbxngnm28vFqxp3wNZs (cherry picked from commit ac343b4) (cherry picked from commit e3aeaf7)
… -> 42.7% reorder_search.py accepted 6 of 66 candidate swaps in round 1 and none in round 2. All six are writes to distinct targets with no read-after-write dependency between them; the one worth checking by hand takes the address of movementRunUpTime while the statement it swaps past writes field199_0x350, so there is no aliasing. The hit rate matches what the tool documents, one move in ten, and confirms how the payoff scales: getFirstSelectedUnitID 25 lines 1 swap +54.9 queueDisbandAndAttackCommand5Params 28 lines 3 swaps +28.6 updateMicroPosition 40 lines 1 swap +8.9 updateUnits 943 lines 6 swaps +1.5 Six times the accepted moves for a thirtieth of the gain. The rate is roughly constant; the value per move falls with function size, because one reordered instruction is a smaller share of the whole. So this lever belongs on small badly-matching functions -- which is exactly what rank_functions.py sorts to the bottom, since it ranks by lines x (1 - match). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
The cherry-picked fix recovers from a wedged mspdbsrv.exe by stopping this worktree''s instance and building again. C1090 has a second cause that recovery cannot reach: the endpoint name itself can wedge, outliving every mspdbsrv.exe that uses it, so retrying into the same name fails forever. Six consecutive builds failed that way here. Killing the server did not help, nor did deleting the PCH and both PDBs; the endpoint was correctly unique, not inherited from the environment, and the toolchain binaries matched a worktree that was building fine at that moment. The same tree then built first try under a different endpoint name, which is what identified the cause. build_quiet.py now takes a second retry on a fresh endpoint, and build_env() accepts an explicit one so a caller can pin it for a session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A4PvfebpUzaMXy93EutBjJ
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.