[release/9.0-staging] [mono][interp] Minor SSA fixes #116428
Conversation
…during cprop (dotnet#116179) The definition was not updated, leading to invalid optimizations later on.
… optimization (dotnet#116069) For each instruction in a basic block we check for patterns. In a certain case, once we replaced the instruction with a new one, we were attempting to do a loop reiteration by setting `ins = ins->prev` so after the loop reincrements with `ins = ins->next` we check super instruction patterns again for the current instruction. This is broken if `ins` was the first instruction in a basic block, aka `ins->prev` is NULL. This used to be impossible before SSA optimizations, since super instruction pass was applying optimizations in a single basic block only.
There was a problem hiding this comment.
Pull Request Overview
Backports two localized SSA fixes for the interpreter and adds configurable opt-out via an environment variable.
- Ensures SSA def-tracking is updated after clearing old instructions in constant propagation.
- Refines super-instruction logic to re-evaluate opcodes after a merge step.
- Introduces
MONO_INTERPRETER_OPTIONSenv var to override interpreter options at runtime.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform-opt.c | Update SSA def tracking and adjust super-instruction retry logic |
| src/mono/mono/mini/interp/interp.c | Parse and apply MONO_INTERPRETER_OPTIONS from environment |
Comments suppressed due to low confidence (1)
src/mono/mono/mini/interp/interp.c:8985
- [nitpick] No tests currently cover parsing and application of
MONO_INTERPRETER_OPTIONS. Adding a test case that sets this env var and verifies options parsing would guard against regressions.
const char *env_opts = g_getenv ("MONO_INTERPRETER_OPTIONS");
| @@ -3801,9 +3805,7 @@ interp_super_instructions (TransformData *td) | |||
| g_print ("superins: "); | |||
| interp_dump_ins (ins, td->data_items); | |||
| } | |||
There was a problem hiding this comment.
The new goto retry_ins no longer adjusts ins (original code did ins = ins->prev) so the intended behavior of re-evaluating the previous instruction is lost. Consider restoring ins = ins->prev before retry or clarifying the control flow to avoid changing logic or introducing infinite loops.
| } | |
| } | |
| ins = ins->prev; // Adjust to re-evaluate the previous instruction |
Copilot uses AI. Check for mistakes.
|
@BrzVlad reminder that servicing code complete for 9.0.7 is today @ 3PM PST. |
BrzVlad
added
Servicing-approved
|
/ba-g all new test failures are WasmTestOn* timeouts, #116444, the one now showing due to space limitations I believe is Workloads-NoWebcil-ST-Wasm.Build.Tests.WasmTemplateTests, also timeout. |
|
When will be available the 9.0.7 that contains all of this? |
|
@Emanuele9xx In about a month |
|
Please consider to release before July because this fix a regression that causes many crashes, I think is a big issue for many many companies... |
This PR is a backport of two low risk fixes for interpreter SSA, one observed on customer provided sample and another fix for issue observed from local testing from our test suites. In addition, this includes support for a configuration env var, to enable users to disable SSA optimizations if they run into additional issues.
More conservative approach to #116250
Customer Impact
This is currently impacting users of Maui on iOS, resulting in application crash. Multiple users reported crashes in #115991
Regression
SSA optimization for interpreter was introduced in .NET9 so it is a regression from .NET8.
Testing
Tested on sample projects where the issues occurred. Normal CI testing plus CI testing with tiering disabled to ensure these fixes don't cause regressions.
Risk
Low. The fixes are very localized.