Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,19 @@ restyling anything large:
`__asm` block rather than C++. Functions are never `__declspec(naked)`: write a normal function, keep the
compiler-generated statements as C++, and refer to parameters, locals and `this` by name. Prettify offsets with
MSVC's struct-member asm syntax (`mov esi, dword ptr [eax]TileMapState.ptr_LogicLayer`) - `::` is not parseable in an
asm operand, so a resolver global cannot be named there.
- a frame pointer plus a `this` spill - the original was built without optimisation. Add `#pragma optimize(, off)`
asm operand, so a resolver global cannot be named there. Where the handwritten block reaches a *global* layer rather
than a `this` field, give it a C++ pointer local and name that local in the asm; the original caches the same bases in
stack slots anyway, so the inner loop still matches. `mov eax, 0` followed by `mov ax, word ptr [..]` is the giveaway
in its own right - that is a hand-rolled `movzx`, which the compiler always emits as one instruction.
Such a block is usually a **macro**: the same instruction stream appears verbatim in several functions and never as a
call, so it belongs in a hand-written `*Asm.hpp` beside them (`NeighbourFlagsAsm.hpp`, `LinkageNeighbourAsm.hpp`).
This has been worth ~45 points per function, three times over.
- a frame pointer plus a `this` spill - the original was built without optimisation, **unless the same function also
reports `mov reg, 0`**. MSVC keeps `ebp` in any function containing an `__asm` block, so for a function with
handwritten assembly the frame pointer says nothing about the optimisation level: reimplement the handwritten part as
inline asm and leave the rest at `/O2`. Checking the prologue settles it - values kept in registers across the entry
tests mean `/O2`. Taking the `/Od` reading instead took `findLinkageBasedPathOrWalkRadius` from 5.1% to 2.8%, where
the same body without the pragma reaches 49.2%. When it really is `/Od`, add `#pragma optimize(, off)`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you had a case that needed \Od?

around the function; inline asm alone does not disable optimisation. Then match the reported **frame size** exactly,
which is usually the single biggest win, because every local displacement shifts otherwise. Merge locals that share a
slot and reproduce dead stores. At `/Od` prefer a nested `if` over an early `continue` (the original inverts the test
Expand Down Expand Up @@ -234,6 +245,13 @@ of the switch leaving the value it should have set uninitialised. Fixing them wa
suspicious dead store is worth decompiling for rather than deleting. Note this is the opposite of the duplicate-tail
pattern below, where our shared block should have been two separate branches - check the predecessor count either way.

Duplicating a `LAB_*` is correct C++ but not automatically a better match, so **measure every `goto` removal**. A
`goto` in the decompiler output usually marks a block the original compiler emitted once and jumped to, and writing
the block out twice makes MSVC emit it twice: duplicating the single shared tail in
`setClimbBasedOnClosestClimbData` - two `if` arms that set a coordinate pair and then share the distance test - was
semantically identical and cost **35 points**. Keep the `goto` where the measurement says the original shared the
block, and record that in the commit remark rather than trading match for style.

Diff patterns that were reliable (more in the cheat sheet):

- Absolute `DAT_*` addresses in the original asm where the source uses `this->` mean the original accessed the global instance.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#include "../PathFindingState.func.hpp"

namespace OpenSHC {
namespace Map {
namespace Navigation {

// FUNCTION: STRONGHOLDCRUSADER 0x004A9760
PathFindingState* PathFindingState::Constructor_PathFindingState()
{
this->field40_0x70 = 0;
this->field41_0x74 = 0;
this->DAT_lWys = 0;
this->notAllAssassinsUnk = 0;
this->DAT_Easy = 0;
this->DAT_Hard = 0;
this->DAT_Test_likely = 0;
this->DAT_Test_gatehouse = 0;
this->searchNonmatchCount = 0;
this->searchMatchCounter = 0;
this->DAT_Ass = 0;
this->calculations = 0;
this->DAT_Mini_spreads = 0;
MACRO_CALL_MEMBER(OpenSHC::Map::Navigation::PathFindingState_Func::clearPathFindingTileMaps, this)();
MACRO_CALL_MEMBER(OpenSHC::Map::Navigation::PathFindingState_Func::clearAllClimbData, this)();
this->searchGeneration = 1;
return this;
}

}
}
}
Loading
Loading