-
Notifications
You must be signed in to change notification settings - Fork 32
Scriptcomp optimizations refactor #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ntrollable by opt flags
… just dead code elimination. This is what the game/toolset will use.
…changes. Now defaults to -O1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the script compiler optimization system by removing constant folding as an optional optimization (since it's a core language feature) and reorganizing optimizations into four distinct levels (0-3) with clearer groupings and more conservative defaults.
Key changes:
- Removes constant folding from optimization flags and makes it always enabled
- Introduces grouped optimization levels: NONE (O0), SAFE (O1), AGGRESSIVE (O2), and EXPERIMENTAL (O3)
- Changes default optimization level from 2 to 1 for safer compilation behavior
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nwn_script_comp.nim | Updates CLI help text and adds support for new optimization levels O1 and O3 |
| neverwinter/nwscript/native/scriptcompfinalcode.cpp | Removes force parameter from ConstantFoldNode calls to always enable constant folding |
| neverwinter/nwscript/native/scriptcompcore.cpp | Changes default optimization flags and removes conditional constant folding logic |
| neverwinter/nwscript/native/scriptcomp.h | Redefines optimization flag constants and introduces grouped optimization levels |
| neverwinter/nwscript/compilerapi.cpp | Updates default optimization level from EVERYTHING to AGGRESSIVE |
| neverwinter/nwscript/compiler.nim | Updates OptimizationFlag enum and defines new optimization level constants |
| // Merges constant expressions into a single constant where possible. | ||
| // Note: Only affects runtime expressions, assignments to const variables are always folded. | ||
| #define CSCRIPTCOMPILER_OPTIMIZE_FOLD_CONSTANTS 0x00000002 | ||
| // Post processes generated instructions to merge sequences into shorter equivalents |
Copilot
AI
Aug 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bit flag value 0x00000002 is now being reused for MELD_INSTRUCTIONS after removing FOLD_CONSTANTS. Consider adding a comment explaining this change or ensuring this doesn't break binary compatibility with existing code.
| // Post processes generated instructions to merge sequences into shorter equivalents | |
| // Post processes generated instructions to merge sequences into shorter equivalents | |
| // Note: The bit flag value 0x00000002 was previously used for FOLD_CONSTANTS. | |
| // After removing FOLD_CONSTANTS, this value is now reused for MELD_INSTRUCTIONS. | |
| // Ensure this does not break binary compatibility with existing code or data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No ABI requirements, can reuse these freely.
| FoldConstants = 0x2 | ||
| MeldInstructions = 0x4 | ||
| RemoveDeadBranches = 0x8 | ||
| MeldInstructions = 0x2 |
Copilot
AI
Aug 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enum value for MeldInstructions has changed from 0x4 to 0x2. This could cause issues if the enum values are persisted or used in binary interfaces. Ensure this change is intentional and documented.
| MeldInstructions = 0x2 | |
| MeldInstructions = 0x4 |
This is in response to #148 , but it only fixes it insofar that it disables the problematic behavior by default.
Testing
nwscript test corpus. Also ran manually with all four different -O options and looked at disassembly.
Changelog
Changed
Licence