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

Elaborate on dataflow outputs for region constraints #1969

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

amandasystems
Copy link

No description provided.

Add a hint about unflatten and an internal link to MIR outlives graphs to advertise them better.
@amandasystems
Copy link
Author

I see Zed snuck in some Markdown normalisation too while it was at it. Sorry about that, let me know if that's inexcusable and I will revert those changes.

@lqd
Copy link
Member

lqd commented May 1, 2024

I will revert those changes

Please do yes, if it's not too hard. It will make review easier.

I'll take a closer look when I have more time, but I noted a couple of things:

  • "With -Z dump-mir-graphviz=yes, you will also get Graphviz files for the outlives constraints" is that the case? Did you maybe mean -Zdump-mir=nll?
  • I don't think these visualizations are from MIR dataflow, so the new paragraph may need to be moved to somewhere more related to borrowck

@amandasystems
Copy link
Author

Please do yes, if it's not too hard. It will make review easier.

Done!

"With -Z dump-mir-graphviz=yes, you will also get Graphviz files for the outlives constraints" is that the case? Did you maybe mean -Zdump-mir=nll?

Huh! Apparently, -Z dump-mir=fn is enough to get those. I assumed it wouldn't drop graphviz files without the Graphviz option, but apparently it does! dump-mir=nll also works and now I'm worried it will dump different graphs.

I don't think these visualizations are from MIR dataflow, so the new paragraph may need to be moved to somewhere more related to borrowck

That's probably true; this should be in the borrowck chapter, maybe? Do you have a suggestion off the top of your head or should I go digging?

@lqd
Copy link
Member

lqd commented May 2, 2024

Not from the top of my head but I will look for one -- but yeah I agree this should be in the borrowck chapter most likely.

Copy link
Member

@lqd lqd left a comment

Choose a reason for hiding this comment

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

(Sorry for taking so long to get back to this.)

So since these are not dataflow outputs, I agree we should add it to the borrow_check chapter.

To do so, I think we should add a dedicated borrow_check/debugging.md page, and have the "Region constraint graphs and their SCCs" section there. We would mention that these come from -Zdump-mir=nll -- and that will give us the occasion to also describe the NLL dumps there in the future.

We can link to that page from the bottom of src/borrow_check.md instead of src/compiler-debugging.md (but we can leave the unflatten paragraph there, it's a good addition next to the graphviz section).

src/mir/dataflow.md Outdated Show resolved Hide resolved
src/mir/dataflow.md Outdated Show resolved Hide resolved
src/mir/dataflow.md Outdated Show resolved Hide resolved
src/compiler-debugging.md Outdated Show resolved Hide resolved
amandasystems and others added 3 commits August 1, 2024 13:38
Co-authored-by: Rémy Rakic <remy.rakic+github@gmail.com>
Co-authored-by: Rémy Rakic <remy.rakic+github@gmail.com>

## See also

The [general instructions on debugging dataflow]() also apply to graphs generated from
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The [general instructions on debugging dataflow]() also apply to graphs generated from
The [general instructions on debugging dataflow](../mir/dataflow.md) also apply to graphs generated from

![A graph showing a small number of strongly connected components on the region-outlives-graph above](../img/scc-graphviz.png)

**Note:** There are implicit edges from `'static` to every region, but those are not rendered
in the region graph to avoid clutter. They _do_ however show up in the SCC graph. This is why there are outgoing edges from `SCC(5)` in the SCC graph that do not seem to have corresponding edges in the region outlives graph above.
Copy link
Member

Choose a reason for hiding this comment

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

This line will need to be wrapped as well.

Comment on lines +256 to +257
This is particularly useful for complicated region outlives graphs from
[MIR Dataflow](mir/dataflow.md).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is particularly useful for complicated region outlives graphs from
[MIR Dataflow](mir/dataflow.md).
This is particularly useful for complicated region outlives graphs from
[the borrow checker](borrow_check/debugging.md).

@jieyouxu jieyouxu added the S-waiting-on-author Status: this PR is waiting for additional action by the OP label Oct 8, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Nov 2, 2024

@lqd btw, you can also edit @amandasystems's branch directly for trivial things like fixing links. We can also try to get this merged so it doesn't drown from merge conflicts. And it's perfectly fine if it still has inaccuracies/omissions, we can always follow-up, it's just docs after all!

@jieyouxu jieyouxu added T-compiler Relevant to compiler team A-dataflow Area: dataflow analysis A-borrow-checker Area: borrow checker A-regions Area: regions labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: borrow checker A-dataflow Area: dataflow analysis A-regions Area: regions S-waiting-on-author Status: this PR is waiting for additional action by the OP T-compiler Relevant to compiler team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants