Skip to content

Fix an SSA bug from the ASG refactoring - #86108

Merged
AndyAyersMS merged 2 commits into
dotnet:mainfrom
SingleAccretion:Fix-Asg-Bug
May 12, 2023
Merged

Fix an SSA bug from the ASG refactoring#86108
AndyAyersMS merged 2 commits into
dotnet:mainfrom
SingleAccretion:Fix-Asg-Bug

Conversation

@SingleAccretion

Copy link
Copy Markdown
Contributor

The SSA renaming code was changed in an incompatible way s.t. memory stores in protected blocks were no longer considered to induce PHIs in the corresponding handlers.

Recording PHIs from analyzable stores is a correctness requirement.
@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels May 11, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

The SSA renaming code was changed in an incompatible way s.t. memory stores in protected blocks were no longer considered to induce PHIs in the corresponding handlers.

Author:SingleAccretion
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

for (GenTree* const tree : stmt->TreeList())
{
if (tree->OperIsSsaDef())
if (tree->OperIsStore() || tree->OperIs(GT_CALL))

@SingleAccretionSingleAccretionMay 11, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In actuality the same problem exists for un-analyzable stores as well. But that is a pre-existing issue.

Reproduction
[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
public staticboolProblem()
{
[MethodImpl(MethodImplOptions.NoInlining)]
staticvoidThrow()
{
_intStatic = 1;
thrownewException();
}
_intStatic = 2;
try
{
Throw();
_intStatic = 2;
}
catch (Exception)
{
if (_intStatic == 2)
{
returntrue;
}
}
returnfalse;
}

Filed #86112 for this.

@SingleAccretion
SingleAccretion marked this pull request as ready for review May 11, 2023 20:30
@SingleAccretion

Copy link
Copy Markdown
ContributorAuthor

No diffs - a small TP regression, but this is a correctness fix.

x86 failure is #35066.

@AndyAyersMSAndyAyersMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this.

@AndyAyersMS
AndyAyersMS merged commit 09ad892 into dotnet:mainMay 12, 2023
@SingleAccretion
SingleAccretion deleted the Fix-Asg-Bug branch May 24, 2023 17:40
@ghostghost locked as resolved and limited conversation to collaborators Jun 23, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@SingleAccretion@AndyAyersMS