Give vararg calls a fixed-arity copy on Lua - #1286
Conversation
On Lua a vararg function kept its `...` parameter and every call packed the arguments into a table, and the inliner refused vararg functions outright. `max`, `min` and `ArrayList.add` are vararg, so every relink in the spatial index allocated four tables and every element added to a list allocated one. Jass has always eliminated varargs before inlining by generating one copy per call arity. The same pass now runs on Lua at the same position, after stack traces and before lowering, with three differences the target needs. Method calls are handled as well as function calls. Classes still exist when this runs on Lua, so `list.add(x)` is an ImMethodCall; the backend already turns a call with exactly one possible implementation into a direct call, and the eliminator does the same for vararg methods. Without it ArrayList.add would never have been specialised. There is no Jass parameter cap. Instead a call with more than 64 vararg arguments keeps the original, which is always still present on Lua. Originals are kept. A vararg function may still be reached through a polymorphic method call, a function reference, or a call above the bound; unreferenced originals go with garbage removal. Measured on the stdlib probe with release flags: table.pack is gone from the output, max and min are max_2 and min_2, and ArrayList.add no longer exists as a function at all because the one-element copy inlines at every call site to a capacity check, one store and one increment.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2dde9b811
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… parameters Two defects in the Lua vararg elimination, both found in review. A copy retargets the function's own references, so a recursive call inside the two-argument copy named that copy. Whenever the recursion uses a different argument count that is wrong: a call of f(7) inside the two-parameter clone invoked itself with a parameter missing. Self-calls now go back to naming the vararg original, and copies are generated to a fixpoint, so a recursive call at an arity nothing else needed still gets a copy of its own. The Lua bound counted source arguments, but the emitted parameter list costs flattened ones: twenty four-field tuples are eighty parameters, past what the target accepts and past what the locals-table fallback can spill, since that cannot spill formal parameters. The bound is now counted after tuple flattening and the constant says parameters rather than arity. A third report, a vararg parameter forwarded into a method call, is not reachable: a vararg function may have only the one parameter, so a receiver cannot be a second one, and the argument does not type as the element type. Both halves are pinned by a test, since the forwarding branch is otherwise only reached from calls this pass generated itself.
Both branches added tests at the same place in LuaBackendAuditTests, which is the whole conflict; both sets are kept.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f361d0a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two ways a generated copy could take over something that belongs only to the original it was copied from. A preserved name is part of the map's Warcraft-facing API, set by @preserveName and by ExecuteFunc. The copy inherits the flag, and because it also shares the original's trace, collectPredefinedNames resets both to the same source name: the emitted Lua then defines that name twice and the later definition wins. ExecuteFunc makes this easy to reach, since it marks its target preserved and emits a zero-argument call, which is exactly what makes a copy. The flag is now dropped from copies on Lua, where the original is retained and keeps the name that external code calls. The other is the reference case of the recursion fix. ReferenceRewritingCopy retargets a function's own references, and the repair visitor undid that for calls but not for function references, so a self reference inside a copy kept naming the copy: registering it as a callback would invoke a fixed-arity body at an arity nobody checked. Those two node types are the only ones the copy retargets that name a function at all, so the visitor now covers the pair rather than the reported half. Lua only, because nothing redirects a reference afterwards and only that target keeps the original; on Jass it is removed and the reference would dangle. Jass keeps both flags and both reference kinds exactly as before.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99f5daee5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A vararg constructor is reached through a generated new_C wrapper that passes its own vararg parameter to construct_C. When a call above the bound keeps that wrapper as the retained original, its body still holds the forwarding call, and the placeholder is one node standing for however many arguments the caller passed. Counting nodes turned it into an arity, so construct_C_1 was generated and the call rewritten to it: every argument after the first was dropped and the constructor ran on one value. Node count is only an arity when no node is a placeholder, so calls that forward one are now left alone. This is Lua-only in effect. The forwarding call survives in the body of a vararg original, and a copy has its placeholder expanded into real parameters long before anything counts them again, so only originals match - which Jass removes and Lua retains. Both the generation and the rewrite loop consult the same predicate. Skipping generation alone would not have been enough: another call can produce a copy at the same node count, and the rewrite would then redirect the forwarding call to it regardless. The existing above-the-bound test used a plain function, which has no wrapper and so never forwards. The new test uses a constructor and fails on the constructed value before this change.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Preserve type arguments when redirecting vararg calls
When a generic vararg returns its type parameter—for example, first<T>(vararg T) called as first<string>("a") inside string concatenation—this replacement discards the call's ImTypeArguments. LuaNativeLowering runs next and identifies string concatenation from each argument's attrTyp(); the redirected call now reports an unresolved ImTypeVar, so the operation is not lowered to concatenation and Lua emits numeric +, causing a runtime error. Preserve or move the original type arguments in both redirectCall and redirectMethodCall.
AGENTS.md reference: AGENTS.md:L217-L222
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Both redirects rebuilt the call with an empty ImTypeArguments. That was right on Jass, where EliminateGenerics runs at phase 2 and nothing generic survives to the vararg phase, but it is an assumption about the pipeline written into a node constructor, and Lua reaches this pass with a different amount erased. Measured before changing anything, since the reported failure is a strong claim: a probe that threw whenever either redirect saw a non-empty type argument list found no hit anywhere in LuaBackendAuditTests, VarargTests, GenericsTests, LuaTranslationTests, GenericsWithTypeclassesTests or StdLibOwnTests. A generic vararg returning its type parameter, consumed by string concatenation, also produces the correct string without this change. So the reported miscompilation does not occur: Lua erases these before the pass rather than carrying them into it. The list is moved across anyway. It costs nothing, it is a no-op while the list is empty, and rebuilding a call should copy what the call had instead of restating a fact about phase order that only holds on one target.
|
Changed in 4df540c, but the reported failure does not occur, and I would rather say that than let a P1 stand unchallenged. I measured it before touching anything. A probe that threw whenever either redirect saw a non-empty That runs in Lua and produces The premise behind the finding is still worth acting on, though, so the list is moved across in both The test is committed as documentation of the scenario rather than as a regression pin, since it passes either way. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
* Stop the local-player barrier from refusing inlining on control taint (#1284) * Optimize Lua array reads and div/mod emission (#1288) * Optimize Lua array reads and div-mod intrinsics Remove typed primitive array normalization, invert legacy assertions to require raw reads, and keep erased-generic normalization intact. Emit raw div/mod primitives directly as Lua operators without helper definitions. * Track Lua numeric intrinsics by IM identity * Give vararg calls a fixed-arity copy on Lua (#1286) * Inline small Lua helpers regardless of popularity (#1289) * Deduplicate Lua callback adapters (#1290) * Deduplicate Lua callback adapters * Preserve renamed Lua callback targets * Bound Lua inlining by register pressure (#1291)
1 participant
What
On Lua a vararg function kept its
...parameter, every call rantable.pack(...), andImInliner.isInlineCandidaterefused vararg functions outright.max,minandArrayList.addare vararg, so every cell relink in the stdlib's spatial index allocated four tables and every element added to a list allocated one.Jass has always run
VarargEliminatorbefore inlining, generating one copy per call arity. The same pass now runs on Lua at the same position intransformProgToLua(afterStackTraceInjector2, beforeLuaNativeLowering), with three target-specific differences.The three differences
list.add(x)is anImMethodCall, not anImFunctionCall. The Lua backend (ExprTranslation.translate(ImMethodCall)) already turns a call with exactly one possible implementation (!isAbstract, implementation present, no sub-methods) into a direct call of that implementation. The eliminator applies the same rule to vararg methods: it generates the copy from the implementation with the receiver as its first argument and replaces theImMethodCallwith anImFunctionCallto the copy. Without thisArrayList.addis never specialised, which the pre-existingoptimizedTupleVarargLoopUsesAttachedScalarLocalstest demonstrated on the first attempt.LUA_MAX_SPECIALISED_VARARG_ARITY = 64(Lua caps a function at 200 locals including parameters). A call above it keeps the original...function.prog.getFunctions().removeIf(IS_VARARG)is Jass-only. On Lua a vararg function may still be reached through a polymorphicImMethodCall, anImFuncRef, or a call above the bound; unreferenced originals go withRemoveGarbage.Jass behaviour is unchanged: the one-argument constructor delegates with
luaTarget = falseand every Jass branch is the old code.Measured
Stdlib-linked probe compiled with
-inline -localOptimizations:table.packin the emitted scriptmax/minalone had 87 call sites)max/minfunction max(...)max_2(a, b),min_2(a, b)ArrayList.addArrayList_add_specialized(this, ...)with a pack and a loopEmitted
SparseSet.addafter this change, for one element:The
(receiver, ...)signatures still in the output are thedispatch_*stubs for polymorphic methods; they forward...without packing and are not Wurst varargs.Tests
New in
LuaBackendAuditTests, alltestLua(true)where they execute:staticArityVarargCallsAreFixedArityOnLua: release-Lua shape, notable.pack, nofunction biggest(...), no call left in the loop.fixedArityVarargLoweringKeepsSemanticsOnLua: runtime parity for zero, one and several arguments, tuple varargs, early return inside the loop, and a vararg class method called directly; asserts no pack in the output.varargCallAboveTheLuaArityBoundKeepsThePackedPath: 150 arguments compile, run, and keep the original.virtuallyDispatchedVarargMethodKeepsThePackedPath: an interface with two vararg implementations dispatches correctly.optimizedTupleVarargLoopUsesAttachedScalarLocals: the assertion thattable.pack(...)is present is inverted; it was pinning the cost.Unchanged and passing:
VarargTests(33, includingvarargAllowsMoreThan31ArgumentsInLuaandtupleVarargPreservesElementGroupingInLua),varargLoopWithBareReturn,localPlayerTaintFlowsThroughVarargLoopValues,luaFunctionRefWrapperForwardsVarargs,ClassesTests.constructor_chaining_vararg.Focused suites:
LuaBackendAuditTests,VarargTests,ClassesTests,LuaTranslationTests,GenericsTests,StdLibOwnTests,OptimizerTests,GenericsWithTypeclassesTests,TypeClassTests,FastHashMapTests: 728 tests, 0 failures. Full suite: 1905 tests, 0 failures.Relation to #1284
Independent; branched from
master. With #1284 also merged,max_2andmin_2inline as well (they are refused there only by the local-player barrier that PR removes). This is Task 3 ofLUA_HOT_PATH_SPEC.md, which lands with #1284.