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: reset max bbnum after failed inline attempt #80958

Merged
merged 2 commits into from
Jan 22, 2023

Conversation

AndyAyersMS
Copy link
Member

And if we renumber blocks in the inlinee's portion of the flow graph, start renumbering from the max bbnum before the inline attempt, rather than the current maximum.

More prep work in anticipation of moving pred list building to happen before (or the root) and during (for inlinees) inlining.

And if we renumber blocks in the inlinee's portion of the flow graph,
start renumbering from the max bbnum before the inline attempt, rather
than the current maximum.

More prep work in anticipation of moving pred list building to happen
before (or the root) and during (for inlinees) inlining.
@ghost ghost assigned AndyAyersMS Jan 20, 2023
@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 Jan 20, 2023
@ghost
Copy link

ghost commented Jan 20, 2023

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

Issue Details

And if we renumber blocks in the inlinee's portion of the flow graph, start renumbering from the max bbnum before the inline attempt, rather than the current maximum.

More prep work in anticipation of moving pred list building to happen before (or the root) and during (for inlinees) inlining.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

More refactoring in preparation for moving pred list building earlier.

@EgorBo PTAL
cc @dotnet/jit-contrib

@AndyAyersMS AndyAyersMS mentioned this pull request Jan 20, 2023
44 tasks
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Reminds me that normally we almost never inline methods with SWITCHes (only with foldable ones) but in some jitstress mode we do that due to extra multipliers so I think I once hit some BBNum related bug there (and fixed)

@AndyAyersMS
Copy link
Member Author

Reminds me that normally we almost never inline methods with SWITCHes (only with foldable ones) but in some jitstress mode we do that due to extra multipliers so I think I once hit some BBNum related bug there (and fixed)

Are you politely suggesting that I should run jit stress?

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Building pri1 tests may be broken?

/__w/1/s/src/tests/reflection/regression/github_25697/25697.cs(9,73): error CS8500: This takes the address of, gets the size of, or declares a pointer to a managed type ('TypedReference') [/__w/1/s/src/tests/reflection/regression/github_25697/25697.csproj] [/__w/1/s/src/tests/build.proj]

@AndyAyersMS
Copy link
Member Author

Libraries stress hitting an assert, probably something I did recerntly (if not in this PR)

Assert failure(PID 28 [0x0000001c], Thread: 41 [0x0029]): Assertion failed 'topBB->bbNum <= botBB->bbNum' in 'FsCheck.TypeClass+FromType@48:Invoke(System.Type):bool:this' during 'Set block order' (IL size 9; hash 0xf9ed1197; Tier0-FullOpts-MinOpts)

    File: /__w/1/s/src/coreclr/jit/optimizer.cpp Line: 4695
    Image: /root/helix/work/correlation/dotne

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jan 21, 2023

Building pri1 tests may be broken?

/__w/1/s/src/tests/reflection/regression/github_25697/25697.cs(9,73): error CS8500: This takes the address of, gets the size of, or declares a pointer to a managed type ('TypedReference') [/__w/1/s/src/tests/reflection/regression/github_25697/25697.csproj] [/__w/1/s/src/tests/build.proj]

Related: Roslyn update in #80850 (partial remediation in #80931)

@AndyAyersMS
Copy link
Member Author

Libraries stress hitting an assert

When we're not optimizing, fgSetBlockOrder requires bbNums in increasing order (or at least fgFirstBB lower than any other BB. We don't have that in this case. Will need a fix.

@AndyAyersMS
Copy link
Member Author

Merging to pick up the jitstress fix.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jan 22, 2023

No diffs in SPMI.

Runtime failure is #80666.
Coreclr jitstress failures are #80666.
Libraries jitstress failures are #80773.

@AndyAyersMS AndyAyersMS merged commit d5d96f4 into dotnet:main Jan 22, 2023
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
And if we renumber blocks in the inlinee's portion of the flow graph,
start renumbering from the max bbnum before the inline attempt, rather
than the current maximum.

More prep work in anticipation of moving pred list building to happen
before (or the root) and during (for inlinees) inlining.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 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 this pull request may close these issues.

2 participants