Stop a merged cycle allocating a parameter slot it already has - #1252
Merged
Conversation
Jass cannot declare a function before it is defined, so functions which call each other in a cycle are compiled into one function taking a choice of which body to run plus the parameters of all of them. A Jass function takes at most 31 parameters, and that merged signature was neither counted nor kept small. Two parameters of the same function are live at once and so cannot share a slot, which is the only constraint. It was being enforced by searching for a free slot from a position which only moves forwards, which enforces more: a parameter matching a slot late in the union puts every parameter after it past everything allocated so far, so it allocates again. The cost lands once per function and accumulates along the cycle. Three functions taking the same five parameters in different orders came out at fifteen slots where nine were needed, and a long enough cycle takes that past 31 while no function in the source is close to it. Remembering which slots the current function has already claimed says the same thing about liveness without imposing an order, and needs one slot per type per position at which some function in the cycle uses that type. Placing call arguments by their parameter's slot rather than walking parameters and slots in step is what makes that legal: the old rewriting relied on slot indices rising with parameter position, which is the constraint being removed. A tuple is passed as one parameter per component, so the check counts flattened arity, and the helper which knew that is now shared with the vararg pass rather than duplicated. The remaining case - a cycle which genuinely needs more than 31 parameters live at once - now fails with the cycle named instead of emitting a function the game rejects at load. Lifting it would mean passing the excess through an array indexed by recursion depth, which is worth doing once something reaches it.
![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: 112a0e8abf
ℹ️ 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".
Comment on lines
221
to
+224
| for (int i = 1; i < newFunc.getParameters().size(); i++) { | ||
| ImVar p = newFunc.getParameters().get(i); | ||
| if (pos < oldArgs.size() && oldToNewVar.get(oldFunc.getParameters().get(pos)) == p) { | ||
| arguments.add(oldArgs.get(pos)); | ||
| pos++; | ||
| } else { | ||
| // use default value | ||
| arguments.add(tr.getDefaultValueForJassType(p.getType())); | ||
| } | ||
| ImExpr argument = argumentBySlot.get(p); | ||
| arguments.add(argument != null ? argument : tr.getDefaultValueForJassType(p.getType())); |
There was a problem hiding this comment.
When a cycle member's parameter order differs from the merged slot order and its call arguments have side effects, this loop moves the expressions themselves into slot order. For example, with merged slots [int, string], a call to a member declared (string, int) as member(nextString(), nextInt()) becomes merged(..., nextInt(), nextString()); the subsequent flattening pass therefore evaluates the mutations in the opposite order and can change program results. Evaluate the original arguments in their original order into temporaries before scattering those values into merged slots.
Useful? React with 👍 / 👎.
Slots are shared by Jass type, so a class reference and an int are interchangeable and so are two tuples with the same components. The slot keeps the IM type of whichever parameter claimed it first, and the other function's body then reads a variable typed as something else. That was possible before and is now the common case, because searching a slot backwards reaches those slots far more often than searching forwards did. It was the one thing about this change I could not tell from reading it, so it is covered rather than assumed: every parameter in the added cycle is a mismatch of that kind, and the values arrive.
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.
Functions which call each other in a cycle cannot be emitted as themselves - Jass has no forward declaration - so they are merged into one function taking a choice of which body to run plus the parameters of all of them. A Jass function takes at most 31 parameters. That merged signature was neither counted nor kept small, which is how the compiler comes to emit a function over the limit while nothing in the source is near it.
What was inflating it
Two parameters of the same function are live at once and so cannot share a slot. That is the only constraint, and it was being enforced by searching for a free slot from a position which only moves forwards - which enforces rather more. A parameter matching a slot late in the union puts every parameter after it past everything allocated so far, so it allocates again. The cost lands once per function and accumulates along the cycle.
Three functions taking the same five parameters in different orders:
n_1,b_1,c_1ands_1are slots the third function could not reach. Reordering arguments between two functions which call each other is ordinary, so this is two functions and one swap, not a corner - and six wasted slots for three functions is enough that a longer cycle clears 31.The change
Remembering which slots the current function has already claimed says the same thing about liveness without imposing an order, and needs one slot per type per position at which some function in the cycle uses that type - the fewest this scheme can use.
Placing call arguments by their parameter's slot, rather than walking parameters and slots in step, is what makes that legal. The old rewriting advanced through the arguments only when the next parameter mapped to the next slot, so it relied on slot indices rising with parameter position - exactly the constraint being removed. Reusing a slot behind the current position without that produces a call with its arguments in the wrong places, which is how I found it.
A tuple is passed as one parameter per component, so the check counts flattened arity rather than parameters; the helper which knew that moved to
ImHelperand the vararg pass now shares it instead of keeping its own copy.What is not fixed
A cycle which genuinely needs more than 31 Jass parameters live at once. With slots shared as tightly as they can be I could not construct one from source, but it is no longer silent: it names the functions in the cycle and says what the count came to. Lifting the limit means passing the excess through an array indexed by recursion depth, which is worth doing once something reaches it.
Tests
CyclicFunctionTestscovers both shapes - two functions with one swap, and a three-function cycle - each run for its result and then checked for the emitted parameter count. The count is asserted exactly, not just against 31: the slack was being spent one duplicate slot at a time, and only a long enough cycle made that visible.Green:
CyclicFunctionTests,SimpleFunctionTests,BugTests,OptimizerTests,NewFeatureTests,PackageTests,ConfigPackageTests,DeterministicChecks,LuaTranslationTests,FastHashMapTests,StdLibOwnTests,ClosureTests,GenericsTests,TypeClassTests,VarargTests.@codex review