JIT: fix issue in cloning loops with trys in handlers #113586
Conversation
Loop cloning with EH had a bug when: * the loop contained a try T0 that was within a handler H1 * the associated try T1 was also within the loop * the entire loop was within another try T2 Here T0's containing try is T2, and its enclosing try index was properly adjusted when the EH table expands as part of cloning. We were doing another index adjustment later which lead to an out of bounds index. Also cleaned up the code that detects which try regions need cloning as in the above we only need to consider T1. When we go to clone it we also will clone H1 and hence T2. Also fixed/added some more dumping, and some new test cases. This fix addresses a problem that came up stress testing dotnet#112998. Since the underlying bug does not require inlining to create the EH setup above, I am fixing it separately.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the loop cloning mechanism for exception handlers and enhances EH-related test coverage by adding new test cases.
- Added new tests for different EH arrangements in loops
- Improved inlining detection in test comments and EH check logic
Comments suppressed due to low confidence (3)
src/tests/JIT/opt/Cloning/loops_with_eh.cs:1133
- Consider adding an inline comment explaining the rationale behind subtracting 130 in this test for clarity and future maintenance.
public static int Test_LTFiTF() => Sum_LTFiTF(data, n) - 130;
src/tests/JIT/opt/Cloning/loops_with_eh.cs:1207
- Consider adding an inline comment explaining why 131 is subtracted in this test to clarify the expected behavior.
public static int Test_TFLTFiTF() => Sum_TFLTFiTF(data, n) - 131;
src/tests/JIT/opt/Cloning/loops_with_eh.cs:1245
- It appears that the [Fact] attribute is commented out for the Test_TFLITFiTF method; please confirm if this test is intentionally disabled or if it should be active.
// [Fact]
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| @@ -2860,7 +2866,7 @@ BasicBlock* Compiler::fgCloneTryRegion(BasicBlock* tryEntry, CloneTryInfo& info, | |||
| // | |||
| if (ebd->ebdEnclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX) | |||
| { | |||
| if (XTnum < clonedOutermostRegionIndex) | |||
| if (ebd->ebdEnclosingTryIndex < clonedOutermostRegionIndex) | |||
There was a problem hiding this comment.
This is the actual fix, we only want to adjust indices within the span of regions we've cloned.
| @@ -2873,7 +2879,7 @@ BasicBlock* Compiler::fgCloneTryRegion(BasicBlock* tryEntry, CloneTryInfo& info, | |||
|
|
|||
| if (ebd->ebdEnclosingHndIndex != EHblkDsc::NO_ENCLOSING_INDEX) | |||
| { | |||
| if (XTnum < clonedOutermostRegionIndex) | |||
| if (ebd->ebdEnclosingHndIndex < clonedOutermostRegionIndex) | |||
|
@dotnet/jit-contrib PTAL Should be no-diff. |
| [Fact] | ||
| public static int Test_TFLTFiTF() => Sum_TFLTFiTF(data, n) - 131; | ||
|
|
||
| public static int Sum_TFLTFiTF(int[] data, int n) |
There was a problem hiding this comment.
This test case has the EH arrangement that triggers the bug.
A try with a loop with a try / finally with a try in the finally.
| // [Fact] | ||
| public static int Test_TFLITFiTF() => Sum_TFLITFiTF(data, n) - 131; | ||
|
|
||
| public static int Sum_TFLITFiTF(int[] data, int n) |
There was a problem hiding this comment.
This is a case like the above but requires inlining to create the proper loop/EH structure; it's what I ran into testing #112998.
Yes, that's what it looks like. |
Though not sure this makes sense, as we haven't bumped the guid recently. Let me retry. It should be no diff. |
Passed on retry, no diff |
Loop cloning with EH had a bug when:
Here T0's containing try is T2, and its enclosing try index was properly adjusted when the EH table expands as part of cloning. We were doing another index adjustment later which lead to an out of bounds index.
Also cleaned up the code that detects which try regions need cloning as in the above we only need to consider T1. When we go to clone it we also will clone H1
and hence T2.
Also fixed/added some more dumping, and some new test cases.
This fix addresses a problem that came up stress testing #112998. Since the underlying bug does not require inlining to create the EH setup above, I am fixing it separately.