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: run instrumentation phase just after importing #47476

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

AndyAyersMS
Copy link
Member

As a prerequisite to enabling efficient instrumentation, add instrumentation
immediately after importing, so the flow graph more closely reflects the IL
level view.

As a prerequisite to enabling efficient instrumentation, add instrumentation
immediately after importing, so the flow graph more closely reflects the IL
level view.
@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 26, 2021
@AndyAyersMS
Copy link
Member Author

Contributes to #46882.

PTAL @dotnet/jit-contrib

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Jit experimental has some known failures in OSR & EH Write Through -- mainly looking at the PGO testing it does here.

@BruceForstall
Copy link
Member

It seems odd to push more things to before the inlining-only and import-only bail-outs. Shouldn't we push more things after those? I guess I don't understand what requires us to do something to inlinees here, versus after then are imported to the inliner.

Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

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

Looks Good

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Jan 26, 2021

It seems odd to push more things to before the inlining-only and import-only bail-outs.

Not sure I understand what you're asking.

We want to instrument early since instrumentation is keyed to IL and the presence of blocks with no IL relationship can confuse things. Currently we can just skip such blocks but when we start looking at spanning tree based instrumentation those blocks can potentially alter the in-tree and non-tree edge sets and it's easier not to have to worry about that at all.

We currently only instrument root methods, but at some point in the future we may instrument inlinees.

I suspect import-only is largely a dead code path now ... ? Though I think @erozenfeld may have used it in his call graph construction prototype. At any rate nothing happens here without BBINSTR being set.

@erozenfeld
Copy link
Member

I suspect import-only is llargely a dead code path now ... ? Though I think @erozenfeld may have used it in his call graph construction prototype.

That's correct, I used import-only mode in the call graph prototype.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

I chatted with @AndyAyersMS and cleared up my questions.

LGTM.

@AndyAyersMS
Copy link
Member Author

Jit experimental test failures are as expected (all pgo tests passed).

@AndyAyersMS AndyAyersMS merged commit 1765d03 into dotnet:master Jan 27, 2021
@AndyAyersMS AndyAyersMS deleted the MoveInstrumentationEarlier branch January 27, 2021 01:41
@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2021
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.

5 participants