Conversation
If the GT_LCLHEAP node's value is unused (no consumer in LIR), remove the node and mark its size arg as unused so it is still evaluated for side effects. This works even when the length is not a constant. Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
…oval - Change LowerLclHeap from void to GenTree* return type to correctly communicate the next node for the lowering loop - Save node->gtNext before removing the node when value is unused, since Remove() sets gtNext to nullptr - Also convert all internal early returns to return node->gtNext - Update the GT_LCLHEAP case in LowerNode to use 'return LowerLclHeap(node)' Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
|
Doesn't liveness remove these? |
given it produces diffs - it doesn't? @copilot please fix liveness to delete unused LCLHEAP nodes instead. |
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Yes. I moved the removal out of lowering and into liveness in 6456523, so unused
Updated in 6456523. |
|
I think the real problem is that we mark It feels inconsistent to be deleting them anyway. If we think we can do that, then we should probably stop marking them with |
|
But it is not clear that is valid -- Edit: well actually, |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@copilot please revert the changes, just remove GTF_EXCEPT from LCLHEAP in importer (unless its argument propagates it). Remove it from OperExceptions. Check everywhere, we can assume StackOverflow is not a side-effect we care about. |
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Agreed. I changed the implementation in c59b003 to stop treating
Went with that simpler direction in c59b003. Since
Done in c59b003. I reverted the liveness-specific removal, removed the importer-side |
|
PTAL @jakobbotsch @dotnet/jit-contrib Diffs I think it also fixes an existing CQ github issue I had in my bookmarks, but I can't find it. |
There was a problem hiding this comment.
Pull request overview
Updates CoreCLR JIT handling of GT_LCLHEAP (localloc) so it’s no longer treated as inherently exception-producing, and adjusts argument reordering logic to account for GT_LCLHEAP explicitly rather than via GTF_EXCEPT.
Changes:
- Stop setting
GTF_EXCEPTon importedGT_LCLHEAPnodes (while keepingGTF_DONT_CSE). - Remove
GT_LCLHEAPfromGenTree::OperExceptionsso it no longer reportsStackOverflowExceptionas an inherent exception. - Update call-argument reordering constraints to explicitly detect
GT_LCLHEAPin argument trees.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/importer.cpp | Removes GTF_EXCEPT from imported GT_LCLHEAP while preserving GTF_DONT_CSE. |
| src/coreclr/jit/gentree.cpp | Removes inherent StackOverflowException reporting for GT_LCLHEAP in OperExceptions. |
| src/coreclr/jit/morph.cpp | Makes argument reordering constraints explicitly detect GT_LCLHEAP rather than relying on GTF_EXCEPT. |
| src/coreclr/jit/lower.cpp | Fixes parameter name in LowerLclHeap comment to match the function signature. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
…ad StackOverflowException flag (#125362) ## Description `GT_LCLHEAP` was previously marked as inherently exception-producing via `GTF_EXCEPT` in the importer, and `OperExceptions` also reported `StackOverflowException` for it. Based on review feedback, this change removes that special exception treatment instead of trying to delete `GT_LCLHEAP` nodes in liveness. As a follow-up cleanup, the now-unused `ExceptionSetFlags::StackOverflowException` enum value is also removed from the JIT exception flag set. ### Changes - **`importer.cpp`** — Stopped marking imported `GT_LCLHEAP` nodes with `GTF_EXCEPT`. The node still gets `GTF_DONT_CSE`. - **`gentree.cpp`** — Removed the `GT_LCLHEAP` case from `GenTree::OperExceptions`, so it no longer reports `StackOverflowException` as an inherent exception side effect. - **`liveness.cpp`** — Reverted the earlier liveness-specific dead-code-elimination handling for `GT_LCLHEAP`. - **`morph.cpp`** — Updated the call-argument reordering logic to check for `GT_LCLHEAP` explicitly in the places where its stack-depth behavior still matters, rather than relying on `GTF_EXCEPT`. - **`utils.h`** — Removed the dead `ExceptionSetFlags::StackOverflowException` enum member after the `GT_LCLHEAP` exception handling change made it unused. - **Behavioral intent** — `GT_LCLHEAP` is no longer treated as inherently exceptional solely because it may grow the stack. Any exception-related behavior now comes from propagated operand flags rather than special casing on the node itself, and the unused stack overflow exception flag has been deleted from the JIT exception set. ## Testing - `./build.sh clr.jit -rc release` - `./build.sh libs -rc release -lc release` - `src/tests/build.sh -Test JIT/Directed/localloc/localloc3_cs_r.csproj x64 Release -priority1` Focused validation succeeded for the affected JIT and localloc test build path. <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
GT_LCLHEAPwas previously marked as inherently exception-producing viaGTF_EXCEPTin the importer, andOperExceptionsalso reportedStackOverflowExceptionfor it. Based on review feedback, this change removes that special exception treatment instead of trying to deleteGT_LCLHEAPnodes in liveness.As a follow-up cleanup, the now-unused
ExceptionSetFlags::StackOverflowExceptionenum value is also removed from the JIT exception flag set.Changes
importer.cpp— Stopped marking importedGT_LCLHEAPnodes withGTF_EXCEPT. The node still getsGTF_DONT_CSE.gentree.cpp— Removed theGT_LCLHEAPcase fromGenTree::OperExceptions, so it no longer reportsStackOverflowExceptionas an inherent exception side effect.liveness.cpp— Reverted the earlier liveness-specific dead-code-elimination handling forGT_LCLHEAP.morph.cpp— Updated the call-argument reordering logic to check forGT_LCLHEAPexplicitly in the places where its stack-depth behavior still matters, rather than relying onGTF_EXCEPT.utils.h— Removed the deadExceptionSetFlags::StackOverflowExceptionenum member after theGT_LCLHEAPexception handling change made it unused.Behavioral intent —
GT_LCLHEAPis no longer treated as inherently exceptional solely because it may grow the stack. Any exception-related behavior now comes from propagated operand flags rather than special casing on the node itself, and the unused stack overflow exception flag has been deleted from the JIT exception set.Testing
./build.sh clr.jit -rc release./build.sh libs -rc release -lc releasesrc/tests/build.sh -Test JIT/Directed/localloc/localloc3_cs_r.csproj x64 Release -priority1Focused validation succeeded for the affected JIT and localloc test build path.
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.