Skip to content

Fix failure with reimportSpillClique=true. #48152

Merged
sandreenko merged 4 commits intodotnet:masterfrom
sandreenko:fixCrossgen2
Feb 12, 2021
Merged

Fix failure with reimportSpillClique=true. #48152
sandreenko merged 4 commits intodotnet:masterfrom
sandreenko:fixCrossgen2

Conversation

@sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Feb 11, 2021 •
edited
Loading

The optimization added in #31677 could cause a failure in tricky conditions.

The test reproduces the failure without crossgen2, it fails on 5.0.

We needed:

  • <DebugType>Full</DebugType>;
  • reimportSpillClique = true, by creating blocks like:
A: leave r4 on the stack;
B: leave r8 on the stack;
C: merge block for A, B, sees one incoming as r4, another as r8, sets reimportSpillClique = true.
  • a CEE_POP that triggers optimization bashToNop from 31677;
  • another stack entry after the poped one to force another spill stack call.

when all the conditions were met we were creating a tree like:

ASG(LCL_VAR void, NOP void)

and failing with assert during local visitor phase, with a noway assert in codegen, then with invalid program exception (in 5.0 release).

The stack when we bashToNop:

dump EE stacks:
depth: 3
               [000044] ------------              *  LCL_VAR   long   V07 tmp7
               [000045] ------------              *  LCL_VAR   long   V08 tmp8
               [000046] ------------              *  LCL_VAR   float  V09 tmp9 <- we would pop it from the current and bash to nop.

block number: 2, depth: 4
               [000044] ------------              *  LCL_VAR   long   V07 tmp7
               [000045] ------------              *  LCL_VAR   long   V08 tmp8
               [000046] ------------              *  LCL_VAR   float  V09 tmp9 <- this is reference saved in `block->bbEntryState`, it will become invalid.
               [000047] ------------              *  LCL_VAR   float  V10 tmp10

so during reimportation, we will try to use the invalid reference during impSpillStackEntry.

Fixes #47308.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 11, 2021
@sandreenko sandreenko marked this pull request as ready for review February 11, 2021 19:59
@sandreenko
Copy link
Contributor Author

sandreenko commented Feb 11, 2021 •
edited
Loading

There are other places that could be cleaned there, for example:

  • runtime/src/coreclr/jit/importer.cpp

    Line 348 in cb5431d

    void Compiler::impSaveStackState(SavedStack* savePtr, bool copy)
    bool copy is always false;
  • in verInitBBEntryState we have a useless memcopy call;
  • the stack on the block boundaries can contain only LCL_VAR or CNST, see impValidSpilledStackEntry, so we could keep them as lclNum or cnst_val and create actual trees only when we need, so we avoid multiply-references to the same GenTree that causes this issue.

but as we discussed with @AndyAyersMS it should be done later when we have more reasons to touch this area.

@sandreenko
Copy link
Contributor Author

sandreenko commented Feb 11, 2021

PTAL @dotnet/jit-contrib

I don't think it meets the bar for 5.0 back-port, what do you think?

@sandreenko
Copy link
Contributor Author

sandreenko commented Feb 11, 2021

/azp run runtime-coreclr crossgen2 outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 11, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Feb 12, 2021
@sandreenko
Copy link
Contributor Author

sandreenko commented Feb 12, 2021

@JulieLeeMSFT it is not an issue, it is a pr. Do we want to move PRs to the project and assigned authors to them?

@sandreenko
Copy link
Contributor Author

sandreenko commented Feb 12, 2021

The failures are unrelated, opened #48204

AndyAyersMS
AndyAyersMS approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Feb 12, 2021

I don't think it meets the bar for 5.0 back-port, what do you think?

I don't think it does (if it were customer reported, then maybe).

@sandreenko sandreenko merged commit 8d02140 into dotnet:master Feb 12, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees

@sandreenko sandreenko

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

.NET Core CodeGen
Archived in project

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

jit asserts in crossgen2 job.

3 participants

@sandreenko @AndyAyersMS @JulieLeeMSFT