Skip to content

JIT: fix inliner profile consistency computation#109714

Merged
AndyAyersMS merged 4 commits into
dotnet:mainfrom
AndyAyersMS:Fix109657
Nov 13, 2024
Merged

JIT: fix inliner profile consistency computation#109714
AndyAyersMS merged 4 commits into
dotnet:mainfrom
AndyAyersMS:Fix109657

Conversation

@AndyAyersMS

@AndyAyersMS AndyAyersMS commented Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

It's possible for the JIT to inline a profiled inlinee into an unprofiled context, and then have a subsequent inline fold a profiled branch. If so we may see a case where the folded edges don't have profile information.

Tolerate this.

Fixes #109657

Re-morphing of a statement during early-prop may mistakenly believe it has altered the flow graph and so invalidates DFS. Value numbering is not set up to handle this and crashes. Since this seems like a rare occurrence, have morph detect if it has really changed the flowgraph and avoid invalidating the DFS when it hasn't.

Fixes #109730

@AndyAyersMS
JIT: fix inliner profile consistency computation
dc3652d 
It's possible for the JIT to inline a profiled inlinee into an unprofiled
context, and then have a subsequent inline fold a profiled branch. If so
we may see a case where the folded edges don't have profile information.

Tolerate this.
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 12, 2024
@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

cc @dotnet/jit-contrib
@amanasifkhalid PTAL

There is some other libraries jitstress issue, so it still may not run cleanly.

@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

/azp run runtime-coreclr libraries-jitstress

@azure-pipelines

azure-pipelines Bot commented Nov 12, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis Bot mentioned this pull request Nov 12, 2024
Open
3 tasks
amanasifkhalid
amanasifkhalid approved these changes Nov 12, 2024
View reviewed changes

@amanasifkhalid amanasifkhalid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

There is some other libraries jitstress issue, so it still may not run cleanly.

Do you know if there's an issue already tracking this? I'm seeing quite a few tests fail with AVs.

@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

LGTM, thanks!

There is some other libraries jitstress issue, so it still may not run cleanly.

Do you know if there's an issue already tracking this? I'm seeing quite a few tests fail with AVs.

I just opened one: #109730

@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

Not sure why build analysis is unhappy here...

@amanasifkhalid

amanasifkhalid commented Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Not sure why build analysis is unhappy here...

I suspect it's because there aren't tracking issues for the timeouts, and opening a meta-issue to capture all timeouts isn't ideal.

Edit: Ah, Build Analysis marked the timeouts as "known" infra issues. Not sure either.

@AndyAyersMS AndyAyersMS mentioned this pull request Nov 12, 2024
Closed
@AndyAyersMS

AndyAyersMS commented Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

I added a fix for the AVs, so let's see if libraries jit stress is now happy.

@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

/azp run runtime-coreclr libraries-jitstress

@azure-pipelines

azure-pipelines Bot commented Nov 12, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

amanasifkhalid
amanasifkhalid approved these changes Nov 12, 2024
View reviewed changes

@amanasifkhalid amanasifkhalid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AV fix LGTM

@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

Jitutils build is failing, wonder if 6.0 packages are no longer findable? 6.0 is out of support now.

Determining projects to restore...
/__w/runtime/runtime/jitutils/src/jit-dasm/jit-dasm.csproj : error NU1102: Unable to find package Microsoft.NETCore.App.Runtime.linux-x64 with version (= 6.0.36)
/__w/runtime/runtime/jitutils/src/jit-dasm/jit-dasm.csproj : error NU1102:   - Found 1562 version(s) in dotnet7 [ Nearest version: 7.0.0-alpha.1.21[41](https://github.com/dotnet/runtime/actions/runs/11804422106/job/32884591447?pr=109714#step:5:42)7.28 ]
/__w/runtime/runtime/jitutils/src/jit-dasm/jit-dasm.csproj : error NU1102:   - Found 174 version(s) in dotnet-public [ Nearest version: 7.0.0-preview.1.22076.8 ]
/__w/runtime/runtime/jitutils/src/jit-dasm/jit-dasm.csproj : error NU1102:   - Found 0 version(s) in dotnet-libraries
/__w/runtime/runtime/jitutils/src/jit-dasm/jit-dasm.csproj : error NU1102:   - Found 0 version(s) in myget-legacy
  Failed to restore /__w/runtime/runtime/jitutils/src/jit-dasm/jit-dasm.csproj (in 1.28 sec).

@AndyAyersMS AndyAyersMS mentioned this pull request Nov 12, 2024
Merged
@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

libraries-jitstress still not clean:

Assert failure(PID 4116 [0x00001014], Thread: 9356 [0x248c]): Assertion failed 'doesVNMatch' in 'System.Collections.Immutable.Tests.ImmutableArrayTest+<ChangeType>d__192`1[int]:MoveNext():ubyte:this' during 'Assertion prop' (IL size 362; hash 0x08aa2d36; FullOpts)

    File: D:\a\_work\1\s\src\coreclr\jit\assertionprop.cpp:1486
    Image: C:\h\w\A3450896\p\dotnet.exe

Opened #109745

@AndyAyersMS

AndyAyersMS commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

handful of Diffs... not sure why yet.

@AndyAyersMS

AndyAyersMS commented Nov 13, 2024

Copy link
Copy Markdown
Member Author

Diffs are because fgConvertBBtoThrowBB also makes the block rare. So revising the fix a bit to preserve this behavior.

@AndyAyersMS AndyAyersMS merged commit 714c5a0 into dotnet:main Nov 13, 2024
@build-analysis build-analysis Bot mentioned this pull request Nov 14, 2024
Closed
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
@AndyAyersMS @mikelle-rogers
JIT: fix inliner profile consistency computation (dotnet#109714)
61912ec 
It's possible for the JIT to inline a profiled inlinee into an unprofiled context, and then have a subsequent inline fold a profiled branch. If so we may see a case where the folded edges don't have profile information.

Tolerate this.

Fixes dotnet#109657

Re-morphing of a statement during early-prop may mistakenly believe it has altered the flow graph and so invalidates DFS. Value numbering is not set up to handle this and crashes. Since this seems like a rare occurrence, have morph detect if it has really changed the flowgraph and avoid invalidating the DFS when it hasn't.

Fixes dotnet#109730
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@amanasifkhalid amanasifkhalid amanasifkhalid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AndyAyersMS AndyAyersMS

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JIT: libraries jitstress AV failures Libraries jitstress: Assertion failed 'target->hasProfileWeight()'

2 participants

@AndyAyersMS @amanasifkhalid