-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[clr-interp] Fix data loss issue at IL merge points #122247
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
base: main
Are you sure you want to change the base?
Conversation
Since the interpreter uses either 32bit or 64 bit floats for values on the stack it is possible to be in a situation where we lose precision in a floating point value at an IL merge point. The architecture of the compiler doesn't have a scheme which would allow the compiler to avoid hitting this problem, so instead, I've built a scheme to record the problematic details, and retry the entire compilation of the method while providing a way to carry the data that certain stack slots need to be more precise than previously expected. This fixes jit\jit64\opt\regress\vswhidbey\481244\foo2_d which tests for this exact scenario In addition, on 64bit platforms we also have some situations where we can silently upgrade to a 64 bit int, this covers that situation too, as well as the case where we merge a native integer and a ByRef so we should treat the result as a ByRef to avoid any cases where we lose track of a GC pointer.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
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 fixes a data loss issue in the CLR interpreter that occurs at IL merge points when different floating-point precisions or integer sizes are involved. The solution implements a retry mechanism that detects type mismatches during compilation and retries with corrected stack type information.
Key changes:
- Adds a new
u64_ptrhash table specialization to track IL merge point stack states across compilation retries - Implements
InterpreterRetryDataclass to manage the retry logic and store override stack type information - Modifies the compilation loop in
compileMethodto support retrying when stack type mismatches are detected - Updates
EmitBBEndVarMovesto detect and handle data-loss conversions (R8→R4, I8→I4, ByRef→I) by triggering retries
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/containers/dn-simdhash-u64-ptr.c | New hash table specialization for uint64_t keys and void* values |
| src/native/containers/dn-simdhash-specializations.h | Adds declarations for the new u64_ptr hash table type |
| src/native/containers/CMakeLists.txt | Registers the new u64_ptr source file in the build |
| src/coreclr/interpreter/simdhash.h | Adds holder class for u64_ptr hash table with RAII semantics, and adds null check/HasValue method to ptr_ptr holder |
| src/coreclr/interpreter/eeinterp.cpp | Implements retry loop in compileMethod to handle compilation failures due to stack type mismatches |
| src/coreclr/interpreter/compiler.h | Adds InterpreterRetryData class and updates InterpBasicBlock constructor to track EH clause indices |
| src/coreclr/interpreter/compiler.cpp | Implements retry logic in AllocBB, updates EmitBBEndVarMoves to detect problematic conversions, and implements retry data storage/retrieval methods |
| } | ||
| else if (interpType == InterpTypeR8 && interpDestType == InterpTypeR4) | ||
| { | ||
| // Data-loss conversion on IL stack merge point. We will need to restart compilation, but for now, emit a conv to continue processing, but update the stack data to reflect the change we need to make. |
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.
Cannot we recompile just the single BB instead, somehow using emitState of the BB and the retry_emit mechanism we have?
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.
Its fairly tricky since we would need to recompile the basic block which set up the initial vars for the target basic block, as well as recompile all of the other basic blocks which have already been generated which branch to this target. I think its much simpler to just recompile it all. If this was a common scenario, sure, yeah, we'd do the more efficient thing. But this should be vanishingly rare, so lets avoid building complex machinery.
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.
FWIW the jit uses "spill cliques" to selectively reimport just the impacted blocks for cases like this. I think that added complexity of that is not worth worrying about, as long as you can find all (or most) of the places in the initial attempt (so that you're not repeatedly retrying).
Since the interpreter uses either 32bit or 64 bit floats for values on the stack it is possible to be in a situation where we lose precision in a floating point value at an IL merge point. The architecture of the compiler doesn't have a scheme which would allow the compiler to avoid hitting this problem, so instead, I've built a scheme to record the problematic details, and retry the entire compilation of the method while providing a way to carry the data that certain stack slots need to be more precise than previously expected.
This fixes jit\jit64\opt\regress\vswhidbey\481244\foo2_d which tests for this exact scenario
In addition, on 64bit platforms we also have some situations where we can silently upgrade to a 64 bit int, this covers that situation too, as well as the case where we merge a native integer and a ByRef so we should treat the result as a ByRef to avoid any cases where we lose track of a GC pointer.