JIT: Spill value before byref address in stsfld #115772
Conversation
We could end up producing trees like ``` ▌ STOREIND ref ├──▌ ADD byref │ ├──▌ CALL help byref CORINFO_HELP_GETDYNAMIC_GCSTATIC_BASE_NOCTOR │ │ └──▌ CNS_INT long 0x7ff84228d6b8 │ └──▌ CNS_INT long 0 Fseq[s_3] └──▌ CALL ref Program:M2():int[] (async) └──▌ CNS_INT ref null ``` where a byref is live across a suspension point.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS @EgorBo |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug where a byref value can be live across a suspension point by ensuring it is spilled appropriately before a stsfld operation. The changes include renaming a variable for clarity in importer.cpp and adding a new condition to spill byref addresses in async methods, along with printing an async flag in gentree.cpp.
- Rename of the variable from check_spill to checkSpill in importer.cpp.
- Added a conditional branch in importer.cpp to spill side effects for byref addresses in async methods.
- Updated gentree.cpp to print an "(async)" suffix when a call is asynchronous.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/importer.cpp | Renamed variable and added a new condition for spilling byref addresses in async methods. |
| src/coreclr/jit/gentree.cpp | Added an indication for asynchronous calls in the debug tree display. |
| @@ -9627,6 +9627,12 @@ void Compiler::impImportBlockCode(BasicBlock* block) | |||
| { | |||
| impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("value for stsfld with typeinit")); | |||
| } | |||
| else if (compIsAsync() && op1->TypeIs(TYP_BYREF)) | |||
There was a problem hiding this comment.
The new condition for spilling byref addresses in async methods always triggers the spill even though the inline comment suggests it may only be necessary if op2 possibly performs an async call. Consider refining this condition in the future to more precisely match the intended necessity for spilling.
Copilot uses AI. Check for mistakes.
|
/ba-g Failure is timeout and build failure fixed by #115767 |
We could end up producing trees like
where a byref is live across a suspension point.
Fix #115671