Compute liveness constraints in location-sensitive polonius #134670

lqd · 2024-12-23T00:41:52Z

This continues the location-sensitive prototype. In this episode, we build the liveness constraints.

Reminder of the approach we're taking: we need variance data to create liveness edges in the forward/backward/both directions (respectively in the cases of covariance, contravariance, invariance) in the localized constraint graph.

This PR:

introduces the holder for that, and for the liveness data in the correct shape: the transpose of what we're using today, "live regions per points".
records use/drop live region variance during tracing
records regular live region variance at the end of liveness
records the correctly shaped live region per point matrix
uses all of the above to compute the liveness constraints

(There's still technically one tiny part of the liveness owl left to do, but I'll leave it for a future PR: we also need to disable the NLL optimization that avoids computing liveness for locals whose types contain a region outliving a free region -- the existing constraints make it effectively live at all points; this doesn't work under polonius)

r? @jackh726 cc @matthewjasper

lqd · 2024-12-23T00:43:10Z

Again this should be gated and deactivated, but let's check if the changes have any impact:

@bors try @rust-timer queue

Compute liveness constraints in location-sensitive polonius This continues the location-sensitive prototype. In this episode, we build the liveness constraints. Reminder of the approach we're taking: we need variance data to create liveness edges in the forward/backward/both directions (respectively in the cases of covariance, contravariance, invariance) in the localized constraint graph. This PR: - introduces the holder for that, and for the liveness data in the correct shape: the transpose of what we're using today, "live regions per points". - records use/drop live region variance during tracing - records regular live region variance at the end of liveness - records the correctly shaped live region per point matrix - uses all of the above to compute the liveness constraints (There's still technically one tiny part of the liveness owl left to do, but I'll leave it for a future PR: we also need to disable the NLL optimization that avoids computing liveness for locals whose types contain a region outliving a free region -- the existing constraints make it effectively live at all points; this doesn't work under polonius) r? `@jackh726` cc `@matthewjasper`

bors · 2024-12-23T00:44:22Z

⌛ Trying commit 47d46ae with merge df62f54...

compiler/rustc_borrowck/src/polonius/liveness_constraints.rs

bors · 2024-12-23T02:32:03Z

☀️ Try build successful - checks-actions
Build commit: df62f54 (df62f54b81cb778616d2f16730ff9e381f66fdec)

rust-timer · 2024-12-23T04:45:38Z

Finished benchmarking commit (df62f54): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 763.813s -> 762.245s (-0.21%)
Artifact size: 330.68 MiB -> 330.70 MiB (0.01%)

compiler/rustc_borrowck/src/polonius/liveness_constraints.rs

jackh726

First commit

compiler/rustc_borrowck/src/polonius/mod.rs

jackh726

Two small thoughts, but r=me with or without addressing them.

jackh726 · 2024-12-27T12:42:53Z

compiler/rustc_borrowck/src/type_check/mod.rs

+    if let Some(polonius_context) = typeck.polonius_context.as_mut() {
+        let num_regions = infcx.num_region_vars();
+        let points_per_live_region = typeck.constraints.liveness_constraints.points();
+        polonius_context.record_live_regions_per_point(num_regions, points_per_live_region);


I wonder instead of using an Option in PoloniusContext, if it's better to just use a different struct for MirTypeckResults...

Yes, however I'd like to do something similar in a follow-up if you don't mind: what we eventually want to do I think is compute the localized constraints earlier (as they should only depend on mir typeck + liveness), and only store loan liveness in the context instead, with the optional debugging data that the polonius mir dump needs.

jackh726 · 2024-12-27T12:49:47Z

compiler/rustc_borrowck/src/polonius/liveness_constraints.rs

-
-        if region.is_bound() || region.is_erased() {
-            // ignore these
+        let Some(region) = self.universal_regions.try_to_region_vid(region) else {


Hmm, so this seems interesting. Where would we be seeing a Bound or Erased region? Seems curious to me that there's some subset of regions that we're essentially just ignoring.

Could be worth adding a comment explaining the cases this might happen.

This came about like so:

I wanted the variance for vars and free regions and could only get it by duplicating the Generalizer's trick with TypeRelation, which unfortunately also yields regions that can't be converted to vids.

so I just ignored these ICE causing kinds and saw no particular changes in the tests (though there are still 6 failures left to diagnose and they could hiding in there): as far as I know there were no live regions that lacked variance due to this. There's also similarly unexpected kinds reaching borrowck, due to issues upstream and/or the ongoing effort of continuing compilation of code as deep as possible, that could be involved. To see whether we should be trying to e.g. recurse after skipping or discharging the binder in a different way that the binders callback is doing maybe, I casually brought it up with Niko who said "seems fine to ignore for now" and we didn't think more of it since then.

Since then I also discovered our test coverage is actually very weak in that area, so I'll ask you what is the correct thing to do here?

Personally, I would prefer to just fail to compile if we ever hit this (as we do for the examples you linked).

Really, I think there's an underlying problem here that we should investigate.

Sure, I'll put the ICEs back in then, and we can recheck and investigate later.

Yeah, for sure we can always come back to it.

Sure, I'll put the ICEs back in then, and we can recheck and investigate later.

Actually maybe not, I forgot there's too many of them and I don't believe they really matter here: we're not ignoring regions in the graph because of the (transparent to reachability) fallback if no variance is found for a region. As for 'erased though, it was in this test that involves GCE amongst others, so imho it's fine to ignore for now.

A fixme like you initially suggested looks best under these circumstances. I'll ping you on zulip to discuss.

A FIXME is fine.

Ok cool, I've pushed a fixme in the last commit, explaining why it's fine to not have variance data for these for now but we still need to look into it. Let me know what you think.

This context struct will hold data to help creating localized constraints: - the live regions, with the shape matching a CFG walk, indexed per point - the variance of these live regions, represented as the direction we'll add the appropriate We also add this structure to the mir typeck to record liveness data, and make it responsible for localized constraint creation.

Following the Generalizer's structure, relating e.g. a type with itself will allow tracking the variance wrt the contained regions.

records the variance of: - use live types - drop live generic args

it missed the index and bounds info

transpose liveness matrix and record live regions at the end of MIR typeck

- add a FIXME when looking for the region variance of unexpected regions - drive-by: fix a doc comment link - drive-by: simplify the variance match using exported variants instead

jackh726 · 2024-12-29T23:53:19Z

@bors r+

bors · 2024-12-29T23:53:21Z

📌 Commit 089c525 has been approved by jackh726

It is now in the queue for this repository.

bors · 2024-12-30T01:58:42Z

⌛ Testing commit 089c525 with merge 8cdc67e...

bors · 2024-12-30T04:45:45Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 8cdc67e to master...

rust-timer · 2024-12-30T06:02:24Z

Finished benchmarking commit (8cdc67e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-4.8%, -1.1%]	3
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -0.8%, secondary -2.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 761.218s -> 760.358s (-0.11%)
Artifact size: 325.50 MiB -> 325.48 MiB (-0.00%)

rustbot assigned jackh726 Dec 23, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 23, 2024

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 23, 2024

compiler-errors reviewed Dec 23, 2024

View reviewed changes

compiler/rustc_borrowck/src/polonius/liveness_constraints.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 23, 2024

lqd force-pushed the polonius-next-episode-4 branch from 47d46ae to 94fa197 Compare December 23, 2024 08:41

compiler-errors reviewed Dec 24, 2024

View reviewed changes

compiler/rustc_borrowck/src/polonius/liveness_constraints.rs Show resolved Hide resolved

lqd force-pushed the polonius-next-episode-4 branch from 94fa197 to 0fd4615 Compare December 24, 2024 15:54

jackh726 reviewed Dec 24, 2024

View reviewed changes

compiler/rustc_borrowck/src/polonius/mod.rs Show resolved Hide resolved

compiler/rustc_borrowck/src/polonius/mod.rs Outdated Show resolved Hide resolved

lqd force-pushed the polonius-next-episode-4 branch from 0fd4615 to f4e292a Compare December 25, 2024 14:41

jackh726 approved these changes Dec 27, 2024

View reviewed changes

lqd added 8 commits December 29, 2024 17:47

add variance recording

de049e6

Following the Generalizer's structure, relating e.g. a type with itself will allow tracking the variance wrt the contained regions.

record variance of use/drop live regions

d1cadf6

records the variance of: - use live types - drop live generic args

record variance of regular live regions

1bea7f9

improve bit_set assertion

cbdac2f

it missed the index and bounds info

finish filling polonius context

6e88db9

transpose liveness matrix and record live regions at the end of MIR typeck

liveness constraints: draw the rest of the owl

93527d2

address review comments

089c525

- add a FIXME when looking for the region variance of unexpected regions - drive-by: fix a doc comment link - drive-by: simplify the variance match using exported variants instead

lqd force-pushed the polonius-next-episode-4 branch from f4e292a to 089c525 Compare December 29, 2024 18:01

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2024

bors merged commit 8cdc67e into rust-lang:master Dec 30, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 30, 2024

lqd deleted the polonius-next-episode-4 branch December 30, 2024 06:39

lqd mentioned this pull request Dec 31, 2024

Scalable Polonius support on nightly rust-lang/rust-project-goals#118

Open

18 tasks

Compute liveness constraints in location-sensitive polonius #134670

Compute liveness constraints in location-sensitive polonius #134670

Uh oh!

Conversation

lqd commented Dec 23, 2024

Uh oh!

lqd commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

bors commented Dec 23, 2024

Uh oh!

This comment has been minimized.

Uh oh!

bors commented Dec 23, 2024

Uh oh!

This comment has been minimized.

rust-timer commented Dec 23, 2024

Overall result: ❌ regressions - no action needed

Uh oh!

Uh oh!

jackh726 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jackh726 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqd Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqd Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqd Dec 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackh726 commented Dec 29, 2024

Uh oh!

bors commented Dec 29, 2024

Uh oh!

bors commented Dec 30, 2024

Uh oh!

bors commented Dec 30, 2024

Uh oh!

Uh oh!

rust-timer commented Dec 30, 2024

Overall result: ❌ regressions - no action needed

Uh oh!

Uh oh!

lqd commented Dec 23, 2024 •

edited

Loading

lqd Dec 27, 2024 •

edited

Loading

lqd Dec 27, 2024 •

edited

Loading

lqd Dec 29, 2024 •

edited

Loading