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: show profile data for dot flowgraph dumps #42657

Merged
merged 2 commits into from
Sep 30, 2020

Conversation

AndyAyersMS
Copy link
Member

Also, tweak layout a bit, and update so that if a method is jitted
multiple times the files don't overwrite one another. Instead a
suffix is added to indicate the kind of jit codegen.

Also, tweak layout a bit, and update so that if a method is jitted
multiple times the files don't overwrite one another. Instead a
suffix is added to indicate the kind of jit codegen.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 23, 2020
@AndyAyersMS
Copy link
Member Author

@CarolEidt PTAL. cc @dotnet/jit-contrib

Sample output (InsertionSort) from a tiered PGO run, after the struct promotion phase. Note there are various inconsistencies in the counts.

  • 5-sided blocks are entries and exits.
  • The dog-eared blocks are BBF_INTERNAL.
  • green edges are lexical backedges
  • blue edges are flow to bbNext

InsertionSortSmall

@AndyAyersMS
Copy link
Member Author

If you dump the FG per phase you'll see that once we've computed edge weights the dumps switch to a normalized view, that is:

InsertionSortNormalized

Might make sense to always show one or the other though I'm not sure we're set up to do that easily.

@AndyAyersMS
Copy link
Member Author

Also very early on there are no edges as the code relies on pred lists. So wondering if I should instead/in addition use the successor iterator to show edges.

(need to look at some EH cases too, suspect handlers will be disconnected).

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Sep 24, 2020

@dotnet/jit-contrib thoughts on the following?

  • remove the xml style dumps (not sure what can parse these)
  • rework to allow (but not yet support) other graph markup formats (dgml?)
  • always use successor iterator instead of chasing flow edges
  • some means for tying in EH flow (possibly edges from start and maybe end of try regions to associated handlers?) I have done fine-grained EH edges in the past and it gets to be overwhelming. Or (for dot) we could use cluster subgraphs to show extent of try regions
  • normalized vs raw counts
  • distinguish synthetic weights from profile data?

@AndyAyersMS
Copy link
Member Author

@dotnet/jit-contrib anyone want to take a look? Feedback on options noted above is welcome.

@CarolEidt
Copy link
Contributor

Might make sense to always show one or the other though I'm not sure we're set up to do that easily

I'd be in favor of always showing a normalized count. I can't imagine it would be terribly difficult, though I can imagine that it might be imperfect - I think that it makes the visualization much easier to reason about.

break;
default:
// These may or may not have an edge to the next block.
// Add a transparent edge to keep nodes ordered.
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I realize that it may not always be the best layout, I find it so much easier to reason about these graphs when the nodes are in sequential order by bbNum. IIRC this was an imperfect mechanism for doing so, but it mostly worked.

Copy link
Member Author

Choose a reason for hiding this comment

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

So you'd like to see a spine of blocks in increasing bbNum order? I can add the invisible edges back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be my preference, unless there's consensus against it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Internal blocks like a loop preheader block have a high number at the are allocated as the highest block number. (Until we renumber the blocks)

Copy link
Contributor

@briansull briansull Sep 29, 2020

Choose a reason for hiding this comment

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

My preference is to have the fall through block always be a short straight line down to the next block:

block A  ------ taken branch
     |
     |
block B  ------- taken branch

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do invisible edges to encourage bbNext order, that will give bbNum order if you've recently renumbered, and bbNext order otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great to me.

@AndyAyersMS
Copy link
Member Author

One issue with always showing normalized counts is that we currently have to do a bunch of work to compute the true method entry count because we don't cleanly handle the case where IL offset 0 is a branch target during instrumentation. I plan to fix this but we need to be a bit careful because we also would like to remain backwards compatible with the legacy IBC scheme.

So we currently have 3 phases of "evolution" of the flow graph with profile data:

  • Importer through Build Preds: raw block weights only, no pred edges
  • Build Preds through Compute Edge weights: raw block weights only, pred egdes but no weights
  • post Compute Edge Weights: normalized block weights, pred lists, edge weights

I intend to have the jit start building a more fully descriptive flow graph earlier, but it will take some work.

But if we can get at the "called count" simply from instrumentation then we can have normalized weights from the start, without needing pred edges or pred edge weights.

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.

LGTM

@AndyAyersMS
Copy link
Member Author

Here's what the above would look like with invisible heavy-weight bbNext edges. @CarolEidt is this what you had in mind?

InsertionSortNew

@CarolEidt
Copy link
Contributor

@CarolEidt is this what you had in mind?

Yes, precisely - thanks!

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@AndyAyersMS AndyAyersMS merged commit 1c4910f into dotnet:master Sep 30, 2020
@AndyAyersMS AndyAyersMS deleted the DotDumpUpdateOnly branch September 30, 2020 23:16
@AndyAyersMS AndyAyersMS mentioned this pull request Oct 24, 2020
54 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

4 participants