JIT: Keep side effects in promotion decomposition #115221
Merged
Conversation
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
If the source is a load from an arbitrary address and the destination is a local that is unused then it may be that we end up creating no loads at all. In that case we should still keep the side effects of the address computation.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @kunalspathak |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #114571 by ensuring that side effects from address computations are preserved even when the resultant load is unused.
- Adds a regression test in Runtime_114571.cs to trigger a NullReferenceException due to a null array access.
- Modifies the JIT promotion decomposition logic in promotiondecomposition.cpp to extract and keep side effects when there are zero uses of the address.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_114571/Runtime_114571.cs | Introduces a regression test that intentionally causes a NullReferenceException to verify behavior. |
| src/coreclr/jit/promotiondecomposition.cpp | Alters the condition to extract side effects when numAddrUses is 0, ensuring side effects are maintained. |
Files not reviewed (1)
- src/tests/JIT/Regression/JitBlue/Runtime_114571/Runtime_114571.csproj: Language not supported
| @@ -588,7 +588,17 @@ class DecompositionPlan | |||
| numAddrUses++; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining why side effects are extracted when numAddrUses equals 0 to clarify that this branch preserves the side effects of the address computation for unused loads.
Suggested change
| // If the address is not used (numAddrUses == 0), we still need to preserve | |
| // any side effects from the address computation. Extract these side effects | |
| // and add them to the statement list to ensure they are not lost. |
Copilot uses AI. Check for mistakes.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
If the source is a load from an arbitrary address and the destination is a local that is unused then it may be that we end up creating no loads at all. In that case we should still keep the side effects of the address computation.
Fix #114571