Eagerly construct bodies of THIR

[LeSeulArtichaut]

Proposal

No longer construct the THIR lazily, layer by layer, and instead construct it eagerly.
This concretely means:

• removing thir::ExprRef/thir::StmtRef and having a full THIR tree (not a "shallow" tree)
• constructing a THIR body can be as simple as calling a build_thir function, without the need for visitors to construct it recursively themselves, simplifying code overall
• we could avoid constructing patterns twice (for MIR building and exhaustiveness checking).

Also, this might allow to do more things on the THIR, like unsafety checking (#402).

(Suggested by @nikomatsakis on Zulip)

FAQ

• Why did we do the streaming approach in the first place?
  • My memory is that I wanted to avoid increasing peak memory usage by constructing Yet Another IR. It was also a bit because I thought it was cool. 😁 --Niko
• So... won't this increase peak memory usage?
  • Potentially, yes. We'll only keep the IR for one function at a time, it won't be stored persistently. This could be done either by "stealing" or by having all processing of THIR occur within one query. I lean mildly towards the second approach, but that may not work out well with exhaustiveness checking, I haven't looked closely at whether this query has additional outputs that couldn't be combined with MIR. --Niko
• Can we make that more certain?
  • Yes! @LeSeulArtichaut is working on a prototype so we can measure the effect.
  • Implementation in Eagerly construct bodies of THIR rust#82495
  • The latest perf run shows many improvements on instruction counts up to -1.8% with some small regressions up to 0.4%. There are however memory regressions especially on stress tests, up to 12.8%.
• What alternatives have you considered?
  • We could do a "streaming approach".
  • We could also build the THIR multiple times, once for each user (this is what we do today, effectively).
• A "streaming approach", what's that?
  • The idea would be to have "callbacks" of a sort. When a HIR expression is "mirrored" into THIR, we would also invoke a callback that would do unsafety checking at the same time.
  • The downside with this idea is that it relies on MIR construction to mirror the entire HIR. But that doesn't feel like a necessary requirement for MIR construction. You could imagine us doing optimizations where we avoid constructing the MIR for dead code, for example, or other things like that.
  • Another downside is that writing the 'unsafety check' in an "inverted" way, where it is getting callbacks for individual THIR nodes and not able to drive the mirroring process itself, may be difficult. I suspect the code would be very confusing. It may not even work, we haven't exhaustively explored this option.
  • Finally, exhaustiveness checking still requires us to build the THIR for all the patterns.
• Why not build THIR multiple times? This is what we do today for exhaustiveness checking, after all, isn't it?
  • Yes, today we build the THIR more than once, but only for patterns. It just seems kind of slow and wasteful to build it multiple times. At the end of the day, execution time seems more important than peak memory usage. That said, it is totally viable for us to prototype the unsafety check using the mirroring API and then test the impact on compilation time, and decide whether to do this step afterwards. That might even be a good idea.

Mentors or Reviewers

@nikomatsakis

Process

The main points of the Major Change Process is as follows:

• File an issue describing the proposal.
• A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
  • Finding a "second" suffices for internal changes. If however you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
  • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
• Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

[rustbot]

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

[nikomatsakis]

@rustbot second

[nikomatsakis]

Whoops, I probably shouldn't have seconded this, as there were some outstanding questions. Not sure how to cancel that, though?

EDIT: I just removed the label for now.

[oli-obk]

@rustbot second

my concerns around peak memory usage have been resolved, we'll look into this during the actual impl, but I don't think anyone is very worried.