diff --git a/AGENTS.md b/AGENTS.md index 16786cf9b..a15665195 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -226,6 +226,22 @@ Recent fixes established additional rules for backend work. Follow these for all requirement: common optimized paths must not retain avoidable compiler-introduced allocation, dispatch, copying, or bookkeeping overhead. +### Lua performance policy + +* **Wurst-emitted constructs are consumed by Wurst code.** Never add runtime coercion, nil guards, + normalisation wrappers or other defensive code to emitted Lua whose justification is that foreign + (non-Wurst) Lua might have mutated an emitted table, array or value. A user who bundles raw Lua that + writes into Wurst-emitted structures owns the result. Typed arrays already carry a metatable that + supplies the typed default; a read of a typed array is a raw table index and nothing else. +* **Leverage Lua-native mechanisms wherever semantics permit.** Prefer a metatable default over a + read-site helper, an operator over a helper call, a fixed-arity function over a `...` pack, and a + direct table over an emulated hashtable. Emulating Jass limitations on Lua needs evidence that the + limitation actually applies there. +* **A compiler-introduced call or allocation on an ordinary typed code path is a defect.** The + optimiser must be able to inline small pure helpers; an analysis barrier that refuses to inline a + function must be justified by what that function does, not by where else it happens to be called. +* The concrete open items and their acceptance criteria are in `LUA_HOT_PATH_SPEC.md`. + ### Jass/Lua feature parity * New language/compiler features must be validated for **both Jass and Lua** backends. diff --git a/LUA_HOT_PATH_SPEC.md b/LUA_HOT_PATH_SPEC.md new file mode 100644 index 000000000..a8b4592ef --- /dev/null +++ b/LUA_HOT_PATH_SPEC.md @@ -0,0 +1,448 @@ +# Lua hot-path emission: four compiler tasks + +Specification for removing compiler-introduced overhead from the Lua backend's emitted code. +Written from reading the release output (`-inline -localOptimizations`, no `-stacktraces`) of the +current compiler (`1.9.0.0-nightly-2-gd1bf06792`, which already contains #1280 and #1282) for the +standard library's `UnitSpatialIndex` and `SpatialPartition` packages, plus the inliner's own +decision log. Every claim below was read off that output, not inferred. + +Do the tasks in the order given. Task 2 is the cheapest and unblocks measuring the rest. + +## Policy this spec implements + +1. **Wurst-emitted constructs are consumed by Wurst code.** The compiler does not guard, coerce or + normalise emitted tables, arrays or values against foreign Lua that might mutate them. A user who + bundles raw Lua that writes into Wurst-emitted tables owns the consequences. Any wrapper, nil + check or coercion whose only justification is "external code could have written here" is a bug, + not a safety feature. This retires the "foreign writes" rationale introduced in #1280. +2. **Lua-native mechanisms over emulation.** Where Lua has a direct construct for what Wurst needs + (a metatable default, a table index, an integer operator, a fixed-arity function), emit that + construct. Do not emit a helper call, a vararg pack, or an IM-level shim as a workaround. +3. **A call in a hot loop is the most expensive thing the emitted code can do**, after a table + allocation. Compiler-introduced calls and allocations on ordinary typed code paths are defects. + +## Evidence + +Inner loop of `spatialIndexBeginQuery` as emitted today, per visited entry: + +```lua +next4 = __wurst_ensureInt(UnitSpatialIndex_nextInCell[idx9]) +temp34 = __wurst_ensureReal(UnitSpatialIndex_lastX[idx9]) +dy1 = (__wurst_ensureReal(UnitSpatialIndex_lastY[idx9]) - center_y) +``` + +`__wurst_ensureInt` is itself two nested calls (`__wurst_rawToNumberInt` then `__wurst_rawToInteger`), +`__wurst_ensureReal` one. That is seven Lua calls per visited entry before any work happens, on +arrays declared with a metatable whose `__index` already returns the typed default. + +Inliner decision log for the same build (`-Dwurst.inliner.log=true`), 1678 call sites: + +| decision / reason | count | +|---|---| +| keep: `local_player_context_barrier` | 577 | +| keep: `native` | 556 | +| inline | 326 | +| keep: `rating_too_high` | 146 | +| keep: `not_in_inlinable_set` | 54 | +| keep: `lua_callback_funcref_barrier` | 17 | + +105 of 546 distinct callees are refused by the local-player barrier. They include `max`, `min`, +`headSlot`, `groupSlot`, `coarseSlot`, `cellAt`, `cellCoordX`, `blockOfCell`, `currentMaxDisplacement`, +`unit_getX`, `rect_getMinX`. None of them touches a client-local native. Section "Task 2" explains why. + +Vararg lowering, `max(vararg int)` as emitted: + +```lua +function max(...) + local __args1 = table.pack(...) +``` + +One table allocation per call. `cellCoordX` calls `max` and `min` once each, so every relink in the +sweep allocates four tables, and `ArrayList.add(vararg T)` allocates one per element added. + +## Ground rules for whoever implements this + +- Follow `AGENTS.md`: failing test first, minimal patch, deterministic iteration order, both + backends validated. Run the focused suites named in each task, then the full suite once at the end. +- **Do not** solve any task with a name-based exclusion (no lists of stdlib function names, no + package-name checks, no `startsWith("__wurst")` tests beyond those that already exist). +- **Do not** keep the removed behaviour behind a flag, run arg, or annotation "just in case". Delete it. +- **Do not** narrow a task to the stdlib functions named here. The fixes are structural and apply to + every program. +- **Evidence over reasoning.** For inliner questions, run with `-Dwurst.inliner.log=true` and read the + `[INLINER]` lines. For emitted-shape questions, read the Lua. The test helper + `LuaBackendAuditTests.compileOptimizedLua` compiles with release flags and returns the source. +- **One task, one branch, one PR**, in the order given. Do not start Task 3 on a branch that still carries Task 1. Commit and PR conventions from `LOOP.md` apply (no AI or co-author references anywhere). +- **Identity, not names.** Where a task says to recognise a compiler-synthesised function (the raw div/mod natives in Task 4), compare against the `ImFunction` instance that `LuaNativeLowering` created and recorded, not its name string. AGENTS.md §7 forbids name comparison for semantic identity; keep a reference on `ImTranslator` the way `ensureIntFunc` is kept. +- When a listed test's assertion encodes the behaviour being removed, invert or delete that + assertion and say so in the commit message. Do not weaken unrelated assertions in the same test. + +--- + +## Task 1: Typed primitive array reads are raw table indexes + +### Current behaviour + +Two places wrap primitive array reads in `__wurst_ensureInt` / `__wurst_ensureReal` / +`__wurst_ensureStr`, or `(x == true)` for booleans: + +1. `LuaNativeLowering.lowerPrimitiveArrayBoundaryEnsure` (`translation/imtranslation/LuaNativeLowering.java`). + Its `visit(ImVarArrayAccess)` wraps **every** rvalue primitive array read in the program. Its + `visit(ImFunctionCall)` additionally wraps array reads passed to natives. The Javadoc says + "Arrays can be visible to foreign Lua/Jass code, so a present value can be malformed". That is the + rationale this spec retires. +2. `ExprTranslation.wrapLuaAtExternalBoundary` (`translation/imtranslation/ExprTranslation.java`) + wraps `ImVarArrayAccess` arguments at native call sites during AST-to-IM translation. + +#1280 exempted class field storage (`X_field_storage[this]`) from this. Package-level and local +arrays still pay. In the probe build, 86 `__wurst_ensureInt` and 6 `__wurst_ensureReal` call sites +remain, every one of them on a typed array read. + +### Why the wrapper is dead weight + +`LuaTranslator.getOrCreatePrimitiveArrayMetatable` gives every primitive-typed array a metatable +whose `__index` returns the typed default (`0`, `0.`, `false`, `""`). `defaultValue` installs it for +globals, locals, and nested arrays (`newDefaultArray`). A read of an unwritten key therefore already +yields the correct default with no call. A written key holds whatever typed Wurst code wrote, which +the type checker guarantees is a value of the declared type. There is no third case. + +`ensureInt` also applies `math.tointeger`. No Wurst integer expression produces a Lua float: +`div` lowers through `//` on integers, `R2I` uses `math.floor`/`math.ceil` which return the integer +subtype for representable values, and bit natives return integers. So removing the coercion changes +no observable value. + +### Required change + +- Delete `lowerPrimitiveArrayBoundaryEnsure` and its call from `LuaNativeLowering.transform`, plus + the helpers that exist only for it (`replaceWithEnsure`, `isExternalBoundary`, `isAlreadyNormalized`, + `isAlreadyNormalizedAccess`, `ensureFunctionFor`, and `callWithStacktrace`/`stacktraceParamIndex` + if nothing else uses them). +- In `ExprTranslation.wrapLuaAtExternalBoundary`, remove the `ImVarArrayAccess` branch. The method + should then be a no-op; delete it and its call sites if so. +- Fix the now-wrong Javadoc on `lua.translation.ExprTranslation.translate(ImVarArrayAccess)`, which + claims reads arrive pre-wrapped. +- **Keep** `ExprTranslation.wrapLua` and the `WurstTypeBoundTypeParam` normalisation. Erased generic + storage genuinely can hold `nil` for a primitive; that path is out of scope and must not regress. + The `ensure*` helper functions stay for it. +- Do not add any replacement flag, annotation, or opt-in. + +### Tests + +Existing assertions that encode the removed behaviour, all in `LuaBackendAuditTests`: + +- `erasedGenericPrimitiveDefaultsPropagateThroughCompositeContexts`: the final assertion with + message "global primitive array reads must remain safe for foreign writes" asserts + `__wurst_ensureInt(Test_values[0])` is present. Invert it: assert the compiled source contains + `return Test_values[0]` and does **not** contain `ensureInt(Test_values`. Keep the count assertion + on `Box_Box_get` unchanged (that is the erased-generic path). +- `seededTypeAssuranceBoundaryFuzz`: `readNormalization` and `arrayArgument` currently assert the + wrapped form for `read()` and for the native call argument. Change both to assert the **raw** + access (`TypeAssuranceFuzz_values[N]`) and assert `__wurst_ensure` is absent from `read`'s body + and from the array argument. Keep the `genericArgument` assertions (erased-generic path) as they + are. Keep the "ordinary typed values must not be normalized" assertion. +- Grep the whole test tree for `ensureInt(`, `ensureReal(`, `ensureStr(`, `ensureBool(` and + `== true)` assertions on array reads and update any others the same way. + +New tests, in `LuaBackendAuditTests`: + +- **Emitted shape.** Compile with `compileOptimizedLua` a package with `int array`, `real array`, + `bool array`, `string array` globals, a local `int array[8]`, and a function that reads each in a + `while` loop and passes one read to a `native`. Assert no `__wurst_ensure` and no `== true)` + appears anywhere in the output except inside functions whose name starts with `__wurst_`. +- **Runtime defaults still hold.** A `test().testLua(true).executeProg()` program that reads never- + written slots of all four primitive array types (including index `0`, a large index, and a slot of + a local sized array) and asserts `0`, `0.`, `false`, `""`; then writes `0`/`false`/`""` explicitly + and reads them back. This pins that the metatable, not the wrapper, was carrying the default. +- **Stacktrace mode.** Compile the same shape with `new RunArgs().with("-lua", "-stacktraces")` and + assert the same absence. The wrapper used to receive a stacktrace argument; make sure nothing + else did. + +Focused suites: `LuaBackendAuditTests`, `LuaTranslationTests`, `LuaTypecastingTests`, +`LuaNativesTests`, `LuaRunnerTests`, `FastHashMapTests`, `StdLibOwnTests`. + +--- + +## Task 2: The local-player inlining barrier must not fire on control taint + +### Root cause, precisely + +`LocalPlayerContextAnalyzer` (`intermediatelang/optimizer/LocalPlayerContextAnalyzer.java`) is a +whole-program, flow-insensitive fact propagation. Three edges combine into the over-approximation: + +1. `indexFunctionCall`: `addEnclosingControlDependency(controlContext, entryControlFact(called))`. + A callee's entry-control fact depends on the control context of **every** call site. A function + body's top-level control context is its own entry-control fact, so this is transitive over the + call graph. +2. `indexElementAfterChildren`, `ImReturn` case: + `addEnclosingControlDependency(controlContext, returnFact(owner))`. A function's RETURN fact + fires whenever its entry control is tainted, regardless of what it returns. +3. `functionInliningIsLocalPlayerSensitive` returns true when `localPlayerDependentReturns.contains(f)`. + +Consequence: any function reachable, through any chain of calls, from inside any +`if GetLocalPlayer() == ...` block anywhere in the program has a tainted RETURN fact and is refused +by the inliner at **every** call site, including ones nowhere near client-local code. In a program +that links the standard library that is most of the call graph. `headSlot(cell, groupId)` returns +`cell * 8 + groupId` and is refused. + +Edges 1 and 2 are correct for the passes that need control facts (`BranchMerger`, +`ConstantAndCopyPropagation`, `LocalMerger`, `TempMerger`, all via `isLocalPlayerDependent`). They +are irrelevant to inlining: substituting a callee body at a call site executes that body under +exactly the control context the call already had. Nothing moves across a client-local boundary. +Those passes run after inlining and re-analyse the inlined program, where the control context is +explicit, so they lose nothing. + +### Required change (implemented on `fix/inliner-local-player-barrier`) + +The barrier answers: does this function call a client-local native directly, or is its **return +value derived from one by the data flow of its own body**, independent of what callers pass in. + +Two simpler rules were tried first and are wrong; do not go back to them: + +- *USE facts only* ("transitively calls a client-local native") breaks `OptimizerTests.testInlineAnnotation`: + with the stdlib linked, `print` reaches `GetLocalPlayer`, so every function that prints stops + inlining. Calling something that uses a client-local value is not the same as producing one. +- *Data-only return facts with the ordinary argument-to-parameter edges* still barriers `max`, `min`, + `headSlot` and `cellCoordX` (75 functions in the probe). The analysis is context-insensitive: a + parameter fact merges the arguments of **every** call site, so one `max(...)` call anywhere with a + client-local argument taints `max`, then everything computed from its result. + +The implemented shape in `LocalPlayerContextAnalyzer`: + +- A second dependency map, `dataDependents`, receives every edge added through `addDependency`. + Control edges (`addEnclosingControlDependency`) and call-site argument-to-parameter edges + (`addCallArgumentDependency`, used in `indexFunctionCall` and `indexMethodCall`) go into the full + graph only. +- `propagateDataFacts()` runs after `propagateFacts()`, walks `dataDependents` from the same sources, + and publishes only RETURN facts into `localPlayerDataDependentReturns`. +- `functionInliningIsLocalPlayerSensitive(f)` is `isClientLocalValueSource(f) || functionsDirectlyUsingLocalPlayer.contains(f) || localPlayerDataDependentReturns.contains(f)`. + +Every other consumer of the analysis (`isLocalPlayerDependent`, `functionUsesLocalPlayer`) is +unchanged and still reads the full graph. + +Measured on the stdlib probe with `-Dwurst.inliner.log=true`: `local_player_context_barrier` went +from 577 call sites on 105 functions to 10 call sites on 5 functions (`init_Player`, +`PingMinimapForPlayer`, `GetPlayableMapRect`, `GetCurrentCameraBoundsMapRectBJ`, `InitMapRects`), each +of which really reaches a client-local native. + +### Tests + +- New, in `OptimizerTests`, Jass output (matches the style of `functionUsingGetLocalPlayerMustNotBeInlined`): + a pure `@inline function slot(integer a, integer b) returns integer` returning `a * 8 + b`, called + once inside `if GetLocalPlayer() == Player(0)` and once in plain code. Assert the `_inl.j` output + contains **no** `call slot(` and no `slot(` at all: both sites inlined. Add a second helper that + wraps `GetLocalPlayer()` and assert it remains a call at both sites. +- New, in `LuaBackendAuditTests` with `compileOptimizedLua`: an `int array` with two `@inline` index + helpers, a query loop, and one unrelated function containing a `GetLocalPlayer()` branch that calls + one of the helpers. Assert the loop body contains the arithmetic inline and no call to either + helper, and assert no `function (` definition survives (garbage removal drops it). +- Existing tests that must keep passing unchanged: `OptimizerTests.functionUsingGetLocalPlayerMustNotBeInlined`, + `localPlayerControlMustPropagateThroughCalledFunctions`, `localPlayerControlMustPropagateIntoFunctionReturns`, + `statementsAfterLocalEarlyReturnMustRemainLocallyControlled`, `branchMergerMustNotHoistAcrossClientLocalConditions`, + every `LocalPlayer*` test in `LuaBackendAuditTests`. + +Focused suites: `OptimizerTests`, `LuaBackendAuditTests`, `LuaTranslationTests`, `InterpreterTests`. + +### Expected effect, to verify with the log + +Re-run a stdlib-linked compile with `-Dwurst.inliner.log=true`. `local_player_context_barrier` must +drop from hundreds to the handful of functions that really call a client-local native. `headSlot`, +`groupSlot`, `cellAt`, `max`, `min` (after Task 3), `__wurst_intDiv`, `__wurst_safe_GetUnitX` and +`unit_getX` must show `decision=inline`. + +--- + +## Task 3: Vararg calls with a static argument count are fixed-arity on Lua + +### Current behaviour + +On Jass, `VarargEliminator` (`translation/imtranslation/VarargEliminator.java`) runs in +`WurstCompilerJassImpl.transformProgToJass` after `StackTraceInjector2` and before inlining. It +generates one copy of each vararg function per distinct call arity, unrolls the `ImVarargLoop`, and +redirects the calls. Varargs never reach the backend. + +On Lua, `transformProgToLua` never runs it. `LuaTranslator` renames the last parameter to `...` and +`lua.translation.StmtTranslation.translate(ImVarargLoop, ...)` emits `table.pack(...)` plus a +`while` loop. `ImInliner.isInlineCandidate` refuses vararg functions. So `max(a, b)` allocates a +table, loops over it, and can never be inlined. + +### Required change (implemented on `lua-fixed-arity-varargs`, #1286) + +`VarargEliminator` gained a target flag, `new VarargEliminator(prog, true)`, and `transformProgToLua` +runs it after `StackTraceInjector2` and before `LuaNativeLowering`, the same relative position Jass +uses. On Lua it differs from the Jass run in three ways: + +- **Method calls are handled too.** On Lua classes still exist when this runs, so `list.add(x)` is an + `ImMethodCall`, not an `ImFunctionCall`. The Lua backend already turns a method call with exactly one + possible implementation (`!isAbstract`, implementation present, no sub-methods) into a direct call of + that implementation, so the eliminator does the same for vararg methods: it generates the copy from the + implementation with the receiver as first argument and replaces the `ImMethodCall` with an + `ImFunctionCall` to the copy. Without this, `ArrayList.add` would never have been specialised. +- **No Jass parameter cap; a Lua arity bound instead.** `LUA_MAX_SPECIALISED_VARARG_ARITY = 64`. A call + with more vararg arguments than that keeps the original `...` function, which is always still present + on this target. +- **Originals are kept.** `prog.getFunctions().removeIf(IS_VARARG)` runs on Jass only. On Lua a vararg + function may still be reached through a polymorphic `ImMethodCall`, an `ImFuncRef`, or a call above + the bound; unreferenced originals are removed by `RemoveGarbage`. + +Measured on the stdlib probe (release flags): `table.pack` occurrences went from 5 to 0 (one per emitted vararg function; the call sites that fed them numbered in the hundreds, `max` and `min` alone 87). `max` and +`min` are emitted as `max_2`/`min_2`. `ArrayList.add` no longer exists as a function at all: the +one-element copy inlines at every call site to a capacity check, one store and one increment. + +The remaining `(receiver, ...)` signatures in the output are the `dispatch_*` stubs for polymorphic +methods; they forward `...` without packing and are not vararg functions in the Wurst sense. + +Two language facts learned while writing the tests: a vararg function may have only the one parameter, +and a vararg parameter cannot be forwarded to another vararg call (`sum(rest)` is a type error). The +eliminator's forwarding branch is therefore reachable only from its own generated copies. + +### Tests + +Existing assertions to update, in `LuaBackendAuditTests`: + +- `optimizedTupleVarargLoopUsesAttachedScalarLocals` asserts `table.pack(...)` is present. Invert: + assert it is **absent**, and assert the specialised `add` has two scalar element parameters and no + loop. Keep `assertFalse(compiled.contains("tupleCopy"))`. +- The `ImVarargLoop` visitor in `compileLuaWithRunArgs` stays; it simply finds no loops. + +Existing tests that must pass unchanged: `VarargTests` (all, including +`varargAllowsMoreThan31ArgumentsInLua` and `tupleVarargPreservesElementGroupingInLua`), +`LuaBackendAuditTests.varargLoopWithBareReturn` (runtime + shape), +`localPlayerTaintFlowsThroughVarargLoopValues`, `ClassesTests.constructor_chaining_vararg`, +`LuaTranslationTests.luaFunctionRefWrapperForwardsVarargs` (that one is about `xpcall` wrappers, not +Wurst varargs, and must be untouched). + +New tests, in `LuaBackendAuditTests`: + +- **Shape.** With `compileOptimizedLua`, a `function biggest(vararg int xs) returns int` called as + `biggest(a, b)` and `biggest(a, b, c)` from a loop. Assert no `table.pack` in the output and no + `function biggest(` definition with `...`. Assert the two-argument call site was inlined to + comparisons (no `biggest` call remains) or, if the inliner rating refuses it, that a + `biggest_2(` and `biggest_3(` pair exists with fixed parameters. +- **Runtime parity.** `test().testLua(true).executeProg()` covering: zero varargs, one, several, + tuple varargs, a vararg function forwarding its varargs to another vararg function, an early + `return` inside the loop, and a vararg class method called directly on a concrete class. Each + asserts the same results the interpreter gives (`testSuccess()`). +- **Fallback.** A call with more arguments than the chosen Lua bound compiles, runs, and still + contains `table.pack` for that function only. +- **Virtual dispatch leftover.** An interface with a vararg method and two implementations, called + through the interface. Compiles and runs; the implementations keep `...`. + +Focused suites: `VarargTests`, `LuaBackendAuditTests`, `LuaTranslationTests`, `ClassesTests`, +`GenericsTests`, `StdLibOwnTests`, `OptimizerTests`. + +--- + +## Task 4: Integer `div`/`mod` lower to operators, not helper chains + +### Current behaviour + +`LuaNativeLowering.lowerDivMod` rewrites `DIV_INT` to `__wurst_intDiv(a, b)`, an IM function whose +body calls `__wurst_rawFloorDivInt(a, b)`, a "native" whose Lua body is `return a // b` +(`lua.translation.LuaNatives`). `MOD_INT` becomes `__wurst_modInt` calling `__wurst_rawFmodInt` +which is `return math.fmod(a, b)`. In the probe build `__wurst_intDiv` was never inlined (local-player +barrier, Task 2), so one `div` was three Lua calls. + +### Required change + +- The three raw natives (`__wurst_rawFloorDivInt`, `__wurst_rawFmodInt`, `__wurst_rawFmodReal`) are + intrinsics at the Lua backend, not functions. In `lua.translation.ExprTranslation.translate(ImFunctionCall, ...)` + (the same place that already pattern-matches the `I2S(1 div 0)` abort trap), translate a call to + `__wurst_rawFloorDivInt` to the binary `//` expression and calls to the two fmod natives to a direct + `math.fmod(a, b)` call expression. Do not emit the function definitions when they are only used as + intrinsics. +- After Task 2, `__wurst_intDiv` and `__wurst_modInt` (size under 20) inline at every call site. + Verify with the log; if `ImInliner` still refuses them for a reason other than the barrier, fix + that reason, do not special-case the names. +- Do **not** touch `WurstOperator.moduloInteger`, the interpreter, or constant folding. AGENTS.md §7 + requires all div/mod semantics to stay centralised; this task changes only how the raw primitive is + spelled in the emitted Lua. +- Leave the `I2S(1 div 0)` abort-trap recognition exactly as it is + (`i2sDivByZeroAbortTrapSurvivesDivModLowering` pins it). + +### Tests + +Existing tests that must pass unchanged: `LuaBackendAuditTests.integerDivModMatchJassSemanticsInLua`, +`integerDivModReferenceSemanticsInInterpreter`, `i2sDivByZeroAbortTrapSurvivesDivModLowering`, +`nonConstantDivModCallsUseSharedHelper`. `divModHelpersAreOmittedWhenUnused` asserts the raw native +"always survives somewhere"; update it to assert the `//` operator or `math.fmod` appears instead, +and that no `__wurst_rawFloorDivInt` function definition is emitted. + +New test, `LuaBackendAuditTests` with `compileOptimizedLua`: a loop doing `x div 8` and `x mod 8` on +runtime values. Assert the loop body contains `// 8` (or the inlined `intDiv` body using `//`) and +`math.fmod`, and no call to `__wurst_raw`. + +Negative-operand semantics are the whole risk here; the runtime parity test +`integerDivModMatchJassSemanticsInLua` already covers `-7 div 2`, `7 mod -2` and friends. Run it under +`testLua(true)`. + +--- + + +--- + +## Task 5 (follow-up, measured after Task 2): the rating formula refuses tiny popular helpers + +With the barrier fixed, the inliner log on the stdlib probe shows the next reason small leaves stay +as calls in hot loops is `rating_too_high`: + +| callee | body | typical decision | +|---|---|---| +| `real_floor` | `toInt` plus a sign correction | `rating_too_high(875.0>=50.0)` | +| `unit_getX` | `return __wurst_safe_GetUnitX(this)` | `rating_too_high(118.0>=50.0)` | +| `unit_getAbilityLevel` | one nil-safe native wrapper | `rating_too_high(252.0>=50.0)` | +| `__wurst_intDiv` | floor-div plus one correction | `rating_too_high(992.0>=100.0)` | +| `__wurst_ensureInt` | two nested coercions | `rating_too_high(1088.0>=50.0)` (gone after Task 1) | + +`ImInliner.getRating` is `size * (callCount - 1)` against a threshold of 50 (100 when an argument is a +constant), with an early "always inline" only when `estimateSize(f) < 20`. `estimateSize` counts every +IM node, so a one-line wrapper around a nil-safe native is already past 20, and any such wrapper with a +handful of callers is refused everywhere. That is backwards for Lua: the cost the formula guards +against is emitted-script size, and duplicating a twenty-node body at each of ten call sites is cheaper +at runtime than ten calls in a loop and negligible in size. + +### Required change + +- Raise the unconditional small-body threshold for the Lua target so that a body consisting of a + single return of one call or one arithmetic expression, with or without a nil guard, always inlines + regardless of call count. Derive the number from `estimateSize` of exactly those shapes (measure + `unit_getX`, `__wurst_safe_GetUnitX`, `real_floor`, `__wurst_intDiv` in the log with a temporary + print, then set the threshold just above the largest), and record the measured sizes in the test. +- Keep the Jass behaviour unchanged unless the same measurement shows the same win there; the map + script size limit is a real constraint on Jass and is not on Lua. +- Do not special-case names. Do not make `@inline` the answer: stdlib authors should not have to + annotate every one-line accessor, and user code will not. + +### Tests + +- `LuaBackendAuditTests`, `compileOptimizedLua`: a one-line nil-safe native wrapper and a one-line + arithmetic helper each called from eight distinct functions. Assert neither helper is called from + any of the eight bodies. +- `OptimizerTests.testInlineAnnotation` and every existing inliner test unchanged. +- Re-run the stdlib probe log: `real_floor`, `unit_getX`, `unit_getAbilityLevel`, `__wurst_intDiv` + must show `decision=inline`. + +## Acceptance for the whole spec + +Compile the standard library's `UnitSpatialIndex` on Lua with release flags (a `withStdLib()` test +that imports `SpatialIndexForUnits` and calls `unitsInRange` is enough) and read +`spatialIndexBeginQuery`. The inner `while` body over a cell chain must be, modulo local names: + +```lua +next4 = UnitSpatialIndex_nextInCell[idx9] +dx1 = (UnitSpatialIndex_lastX[idx9] - center_x) +dy1 = (UnitSpatialIndex_lastY[idx9] - center_y) +cachedDistSq = ((dx1 * dx1) + (dy1 * dy1)) +``` + +with no `__wurst_ensure`, no `table.pack`, no call to any `@inline` leaf, and `cellCoordX` reduced to +arithmetic plus at most one `R2I`/`math.floor` call. The `SpatialPartition` query must read +`SpatialPartition_cellHead[((rowBase + cx) * 8) + groupId5]` directly. + +Then run the full suite once. + +## Measurement note for stdlib authors + +`wurst_run.args` in a generated project defaults to `-stacktraces` and no `-inline`. Every emitted +function then pays `wurst_stack` bookkeeping on entry and exit, and no leaf is inlined. Benchmarks of +emitted code that are meant to inform stdlib design must use `-inline -localOptimizations` without +`-stacktraces`, or they measure the debug configuration. diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java index 28e3b120d..da3eba241 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java @@ -75,11 +75,16 @@ public final class LocalPlayerContextAnalyzer { Collections.newSetFromMap(new IdentityHashMap<>()); private final Set functionsDirectlyUsingLocalPlayer = Collections.newSetFromMap(new IdentityHashMap<>()); + /** Functions whose return value is derived from a client-local value by data flow alone. */ + private final Set localPlayerDataDependentReturns = + Collections.newSetFromMap(new IdentityHashMap<>()); private final Set indexedElements = Collections.newSetFromMap(new IdentityHashMap<>()); private final Set activeFacts = Collections.newSetFromMap(new IdentityHashMap<>()); private final Map> dependents = new IdentityHashMap<>(); + /** The subset of {@link #dependents} reached without any control edge. */ + private final Map> dataDependents = new IdentityHashMap<>(); private final Map variableFacts = new IdentityHashMap<>(); private final Map returnFacts = new IdentityHashMap<>(); private final Map useFacts = new IdentityHashMap<>(); @@ -143,11 +148,20 @@ public boolean functionUsesLocalPlayer(ImFunction function) { && (isClientLocalValueSource(function) || functionsUsingLocalPlayer.contains(function)); } + /** + * Whether inlining this function must be refused: it is a client-local native, calls one + * directly, or returns a value derived from one by data flow. Control taint is deliberately not + * consulted. A function reachable from a client-local branch has a tainted return fact, but + * inlining substitutes its body at the call site, where it runs under exactly the control the + * call already had, so nothing crosses a boundary. The passes which do move code + * ({@link BranchMerger}, {@link TempMerger}, ...) run after inlining and re-analyse the inlined + * program, where that control is explicit. + */ public boolean functionInliningIsLocalPlayerSensitive(ImFunction function) { return function != null && (isClientLocalValueSource(function) || functionsDirectlyUsingLocalPlayer.contains(function) - || localPlayerDependentReturns.contains(function)); + || localPlayerDataDependentReturns.contains(function)); } public boolean isLocalPlayerDependent(ImVar variable) { @@ -166,6 +180,7 @@ private void analyze(ImProg prog) { analyzeFunctions(classes.get(i).getFunctions()); } propagateFacts(); + propagateDataFacts(); } private void analyzeFunctions(List functions) { @@ -410,13 +425,13 @@ private void indexFunctionCall(ImFunctionCall call, ImFunction owner, Object con int argumentCount = arguments.size(); int positionalCount = Math.min(argumentCount, fixedParameterCount); for (int i = 0; i < positionalCount; i++) { - addDependency(arguments.get(i), + addCallArgumentDependency(arguments.get(i), variableFact(calledParameters.get(i))); } ImVar varargParameter = varargParameter(called); if (varargParameter != null) { for (int i = fixedParameterCount; i < argumentCount; i++) { - addDependency(arguments.get(i), + addCallArgumentDependency(arguments.get(i), variableFact(varargParameter)); } } @@ -450,9 +465,9 @@ private void indexMethodCall(ImMethodCall call, ImFunction owner, Object control List parameters = implementation.getParameters(); for (int i = 0; i < parameters.size(); i++) { ImVar parameter = parameters.get(i); - addDependency(receiver, variableFact(parameter)); + addCallArgumentDependency(receiver, variableFact(parameter)); for (int j = 0; j < arguments.size(); j++) { - addDependency(arguments.get(j), variableFact(parameter)); + addCallArgumentDependency(arguments.get(j), variableFact(parameter)); } } } @@ -460,7 +475,8 @@ private void indexMethodCall(ImMethodCall call, ImFunction owner, Object control private void addEnclosingControlDependency(Object controlContext, Object dependent) { if (controlContext != null) { - addDependency(controlContext, dependent); + // A control edge: present in the full graph only, never in the data graph. + dependents.computeIfAbsent(controlContext, ignored -> new ArrayList<>()).add(dependent); } } @@ -550,9 +566,18 @@ private void addLocalPlayerSource(ImFunction function) { } private void addDependency(Object dependency, Object dependent) { - dependents.computeIfAbsent(dependency, - ignored -> new ArrayList<>()) - .add(dependent); + dependents.computeIfAbsent(dependency, ignored -> new ArrayList<>()).add(dependent); + dataDependents.computeIfAbsent(dependency, ignored -> new ArrayList<>()).add(dependent); + } + + /** + * A call-site argument flowing into a callee parameter. Present in the full graph only: the data + * graph answers what a function computes from its own body, so it must not merge the arguments + * of every caller into the parameter. With that merge, one client-local argument to a shared + * helper such as {@code max} would taint the helper and everything computed from its result. + */ + private void addCallArgumentDependency(Object argument, Object parameterFact) { + dependents.computeIfAbsent(argument, ignored -> new ArrayList<>()).add(parameterFact); } private void propagateFacts() { @@ -571,6 +596,36 @@ private void propagateFacts() { } } + /** + * Second pass over the data-only graph. Publishes just the RETURN facts, which is what the + * inlining barrier needs: whether a return value is derived from a client-local value regardless + * of where the function happens to be called from. + */ + private void propagateDataFacts() { + Set reached = Collections.newSetFromMap(new IdentityHashMap<>()); + Deque worklist = new ArrayDeque<>(); + for (Object source : sourceFacts) { + if (reached.add(source)) { + worklist.addLast(source); + } + } + while (!worklist.isEmpty()) { + Object fact = worklist.removeFirst(); + if (fact instanceof Fact typedFact && typedFact.kind == FactKind.RETURN) { + localPlayerDataDependentReturns.add((ImFunction) typedFact.subject); + } + List factDependents = dataDependents.get(fact); + if (factDependents != null) { + for (int i = 0; i < factDependents.size(); i++) { + Object dependent = factDependents.get(i); + if (reached.add(dependent)) { + worklist.addLast(dependent); + } + } + } + } + } + private void activateFact(Object fact, Deque worklist) { if (activeFacts.add(fact)) { publishFact(fact); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java index 00fbd7f68..ac17f6b75 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java @@ -1988,6 +1988,50 @@ public void localPlayerEffectfulBooleanOperandSurvivesOptimization() { compiled.indexOf("localProbe()", definitionOrCall + 1) >= 0); } + /** + * The inliner used to refuse every function whose return fact the local-player analysis had + * marked, and that fact fires for anything reachable from a client-local branch anywhere in the + * program. In a stdlib-linked map that is most of the call graph, so pure index arithmetic in + * hot loops stayed as calls. Only functions which transitively invoke a client-local native are + * an inlining barrier. + */ + @Test + public void pureHelpersReachableFromLocalPlayerBranchInlineIntoLuaHotLoops() { + String compiled = compileOptimizedLua( + "pureHelpersReachableFromLocalPlayerBranchInlineIntoLuaHotLoops", + "type player extends handle", + "package Test", + "@extern native GetLocalPlayer() returns player", + "@extern native Player(integer i) returns player", + "native consume(int i)", + "native consumePlayer(player p)", + "int array cells", + "int offset = 0", + "@inline function slotOf(int cell, int group) returns int", + " return cell * 8 + group", + "@inline function chainHead(int cell, int group) returns int", + " return cells[slotOf(cell, group)]", + "@inline function localWrapper() returns player", + " return GetLocalPlayer()", + "@noinline function query(int group)", + " var cell = 0", + " while cell < 16", + " consume(chainHead(cell, group))", + " cell++", + "init", + " if GetLocalPlayer() == Player(0)", + " consume(slotOf(offset, 1))", + " consumePlayer(localWrapper())", + " query(2)" + ); + + assertFunctionBodyContains(compiled, "query", "slotOf", false); + assertFunctionBodyContains(compiled, "query", "chainHead", false); + assertFunctionBodyContains(compiled, "query", "* 8", true); + assertTrue("a wrapper which itself calls GetLocalPlayer must stay an explicit call", + compiled.contains("consumePlayer(localWrapper())")); + } + @Test public void localPlayerTaintFlowsThroughVarargLoopValues() { String compiled = compileOptimizedLua( diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java index f37b83c95..fc6bb5ade 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java @@ -2285,6 +2285,44 @@ public void functionUsingGetLocalPlayerMustNotBeInlined() throws Exception { "transitive GetLocalPlayer wrappers must remain explicit calls"); } + /** + * The local-player analysis marks a function's return fact whenever the function is reachable + * from a client-local control region, transitively over the call graph. That fact is right for + * the passes which move code across control boundaries and wrong as an inlining barrier: + * substituting a body at a call site runs it under exactly the control the call already had. + * A pure helper called once under a GetLocalPlayer branch must still inline everywhere, while a + * wrapper which itself calls GetLocalPlayer must stay an explicit call. + */ + @Test + public void pureHelperReachableFromLocalPlayerBranchIsStillInlined() throws Exception { + test().lines( + "type player extends handle", + "package test", + "@extern native GetLocalPlayer() returns player", + "@extern native Player(integer i) returns player", + "native consume(integer i)", + "native consumePlayer(player p)", + "integer offset = 0", + "@inline function slot(integer a, integer b) returns integer", + " return a * 8 + b", + "@inline function currentPlayer() returns player", + " return GetLocalPlayer()", + "init", + " if GetLocalPlayer() == Player(0)", + " consume(slot(offset, 1))", + " consume(slot(offset, 2))", + " consumePlayer(currentPlayer())" + ); + + String inlined = Files.toString( + new File("test-output/OptimizerTests_pureHelperReachableFromLocalPlayerBranchIsStillInlined_inl.j"), + Charsets.UTF_8); + assertFalse(inlined.contains("slot("), + "a pure helper must inline at every call site, including the one under the local-player branch"); + assertTrue(inlined.contains("call consumePlayer(currentPlayer())"), + "a wrapper which calls GetLocalPlayer itself must remain an explicit call"); + } + @Test public void branchMergerMustNotHoistAcrossClientLocalConditions() throws Exception { test().lines(