JIT: Copy loop memory dependence recursively on hoisting#116068
JIT: Copy loop memory dependence recursively on hoisting#116068jakobbotsch merged 1 commit intodotnet:mainfrom
Conversation
When hoisting we were cloning the full tree but only copying memory dependence of the root node. Ensure we copy it for the full tree.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #115109 by ensuring loop memory dependencies are copied recursively when hoisting expression trees. It also streamlines statement insertion and adds a regression test to validate the fix.
- Simplify hoisted statement insertion in
optPerformHoistExprusingfgInsertStmtAtEnd - Enhance
optCopyLoopMemoryDependenceto recurse through all tree operands - Add a JIT regression test project and C# test (
Runtime_115109) to cover the full-tree dependence copy
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_115109/Runtime_115109.csproj | New test project (missing framework target) |
| src/tests/JIT/Regression/JitBlue/Runtime_115109/Runtime_115109.cs | Generated fuzz test to verify memory-dependence cloning |
| src/coreclr/jit/optimizer.cpp | Refactored hoist insertion and recursive memory-dependence copy |
| @@ -0,0 +1,8 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <Optimize>True</Optimize> | |||
There was a problem hiding this comment.
The project file is missing a <TargetFramework> entry, so it may fail to build. Please add something like <TargetFramework>net7.0</TargetFramework> under the PropertyGroup.
| <Optimize>True</Optimize> | |
| <Optimize>True</Optimize> | |
| <TargetFramework>net7.0</TargetFramework> |
| public static int TestEntryPoint() | ||
| { | ||
| s_10 = [Vector128.CreateScalar(0).AsVector()]; | ||
| bool vr5 = 0 <= M10(); |
There was a problem hiding this comment.
[nitpick] The variable vr5 is assigned but never used; consider removing it or using the result directly in the return or an assertion to make its purpose clear.
| bool vr5 = 0 <= M10(); | |
| Assert.True(0 <= M10(), "Expected M10() to return a value greater than or equal to 0."); |
There was a problem hiding this comment.
Fair point, but I don't think it's worth rerunning CI.
| // This requires 'toTree' to be in its own statement | ||
| // |
There was a problem hiding this comment.
[nitpick] The remark about 'toTree' needing to be in its own statement is unclear. It would help to clarify the precondition (e.g., both trees must be part of the same statement sequence) or remove if not needed.
| // This requires 'toTree' to be in its own statement | |
| // | |
| // 'toTree' must be part of a separate statement sequence to ensure proper | |
| // traversal and mapping in the loop memory dependence analysis. |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM.
Alternative idea: make this be an optional action for gtCloneExpr, since it has old and new nodes in hand ... (and do likewise for optRecordSsaUses).
It means everyone gets to pay for it. I removed optional actions from |
When hoisting we were cloning the full tree but only copying memory dependence of the root node. Ensure we copy it for the full tree.
Fix #115109