Skip to content
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

JIT: SSA does not add proper phi args for second pass EH successors #94954

Closed
jakobbotsch opened this issue Nov 18, 2023 · 1 comment · Fixed by #94955
Closed

JIT: SSA does not add proper phi args for second pass EH successors #94954

jakobbotsch opened this issue Nov 18, 2023 · 1 comment · Fixed by #94955
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Nov 18, 2023

SSA only looks at the full set of EH successors as part of the "end-of-block" successor iteration. When it sees definitions inside the block, it only looks for enclosing handlers. For example:

private static void Foo()
{
    int x = 123;
    try
    {
        try
        {
            throw new Exception();
        }
        finally
        {
            Console.WriteLine(x);
        }
    }
    catch (Exception) when (Bar(x = 50, Throw(x), x = 123))
    {
    }
}

private static bool Bar(int x, int y, int z)
{
    return true;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Throw(int x)
{
    throw new Exception();
}

The x = 50 here needs to induce a phi arg in the enclosed finally handler, but it doesn't. We effectively get a PHI that indicates the only possible value for x is 123 inside the enclosed finally, but this program enters the finally block with x=50.
It does not cause silent bad codegen here because there is a cycle in the flow-graph between the enclosed finally and the filter, so VN isn't able to deduce x = 123 from the phis.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 18, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 18, 2023
@ghost
Copy link

ghost commented Nov 18, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

SSA only looks at the full set of EH successors as part of the "end-of-block" successor iteration. When it sees definitions inside the block, it only looks for enclosing handlers. For example:

private static void Foo()
{
    int x = 123;
    try
    {
        try
        {
            throw new Exception();
        }
        finally
        {
            Console.WriteLine(x);
        }
    }
    catch (Exception) when (Bar(x = 50, Throw(x), x = 123))
    {
    }
}

private static bool Bar(int x, int y, int z)
{
    return true;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Throw(int x)
{
    throw new Exception();
}

The x = 50 here needs to induce a phi arg in the enclosed finally handler, but it doesn't. We effectively get a PHI that indicates the only possible value for x is 123 inside the enclosed finally, but this program enters the finally block with x=50.
It does not cause silent bad codegen here because there is a cycle between the enclosed finally and the filter, so VN isn't able to deduce x = 123 from the phis.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch self-assigned this Nov 18, 2023
@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Nov 18, 2023
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Nov 18, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Nov 18, 2023
When we saw a def we were only adding phi args to enclosing handlers,
but this misses the special case of throws inside filters, where control
can flow to enclosed handlers.

Fix dotnet#94954
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 18, 2023
jakobbotsch added a commit that referenced this issue Nov 21, 2023
When we saw a def we were only adding phi args to enclosing handlers,
but this misses the special case of throws inside filters, where control
can flow to enclosed handlers.

Fix #94954
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant