always resolve to universal regions if possible #108121

aliemjay · 2023-02-16T13:01:11Z

RegionConstraintCollector::opportunistic_resolve_var, which is used in canonicalization and projection logic, doesn't resolve the region var to an equal universal region. So if we have equated 'static == '1 == '2, it doesn't resolve '1 or '2 to 'static. Now it does!

Addresses review comment #107376 (comment).

r? @lcnr

bors · 2023-02-16T20:20:48Z

☔ The latest upstream changes (presumably #108020) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr · 2023-02-17T08:04:35Z

that merge conflict is unfortunate 😅

r=me after perf

Give a more clear name.

Bless tests and show an introduced unsoundness related to exits<'a> { forall<'b> { 'a == 'b } }. We now resolve the var ?a in U0 to the placeholder !b in U1.

aliemjay · 2023-03-05T10:30:34Z

The original change introduced a new unsoundness (see last commits). It is caused by resolving a region variable to a placeholder region from a universe that it can't name. I originally thought this is not a problem because region resolution doesn't rely on unification_table and we would eventually have a universe error, but it tuned out that canonicalization uses opportunistic resolution on query region constraints opening a loophole in this logic.

Theoretically, it is a preexisting bug because in the previous setup we could resolve a variable to another variable of higher universe, and thus allowing it to name more regions, but I can't think of an example where we unify two region variables from different universes inside a canonical query in order to exploit this.

lcnr · 2023-03-06T09:27:19Z

@bors try @rust-timer queue

I am unhappy with bfd3501 as it feels fairly brittle. Can look into this myself at some point, but how difficult is it to 1. eagerly error when unifying a region with a placeholder from a higher universe and 2. when unifying 2 regions 'a and 'b, set their universes to min(uv('a), uv('b)).

bors · 2023-03-06T09:27:31Z

⌛ Trying commit 095b5fa with merge 6296cbeaecfc306187cacaad62bbb14d77d96ded...

bors · 2023-03-06T11:56:18Z

☀️ Try build successful - checks-actions
Build commit: 6296cbeaecfc306187cacaad62bbb14d77d96ded (6296cbeaecfc306187cacaad62bbb14d77d96ded)

bors · 2023-03-06T11:56:18Z

☀️ Try build successful - checks-actions
Build commit: 6296cbeaecfc306187cacaad62bbb14d77d96ded (6296cbeaecfc306187cacaad62bbb14d77d96ded)

rust-timer · 2023-03-06T13:58:54Z

Finished benchmarking commit (6296cbeaecfc306187cacaad62bbb14d77d96ded): comparison URL.

Overall result: ✅ improvements - 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	2
Improvements ✅ (primary)	-0.6%	[-1.1%, -0.2%]	4
Improvements ✅ (secondary)	-1.6%	[-2.6%, -0.6%]	18
All ❌✅ (primary)	-0.6%	[-1.1%, -0.2%]	4

Max RSS (memory usage)

Results

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)	3.5%	[2.5%, 4.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[-2.9%, 4.6%]	3

Cycles

Results

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%	[-3.3%, -1.8%]	7
All ❌✅ (primary)	-	-	0

aliemjay · 2023-03-06T16:29:14Z

I am unhappy with bfd3501 as it feels fairly brittle. Can look into this myself at some point, but how difficult is it to 1. eagerly error when unifying a region with a placeholder from a higher universe and 2. when unifying 2 regions 'a and 'b, set their universes to min(uv('a), uv('b)).

I don't see a problem with the current approach. unification_table stores what vars/values we unified and only when we use it for anything useful we should concern ourselves with universes. I think the alternative approach would be unnecessarily painful and probably a perf hit.

Anway, I tried to implement alternative approach in https://github.com/aliemjay/rust/tree/resolve-var-region-alt but I don't really like it especially how the universes of region variables change during inference.

aliemjay · 2023-03-07T16:06:19Z

cc #108867

lcnr

looked a bit more into this and also at #108867

I think I agree with you now that this approach is nicer but thought of a way to make this harder to misuse by accident in the future.

r=me after this nit

lcnr · 2023-03-08T07:43:56Z

compiler/rustc_infer/src/infer/region_constraints/mod.rs

-            _ => region,
+        let mut ut = self.unification_table_mut(); // FIXME(rust-lang/ena#42): unnecessary mut
+        let root_vid = ut.find(vid).vid;
+        let resolved = ut.probe_value(root_vid).0.unwrap_or_else(|| tcx.mk_re_var(root_vid));


this is the only use of probe_value for regions 🤔 so yeah I agree that this is fine.

Can you add a comment to UnifiedRegion that the assigned value may not be nameable by the region var?

Or even better, change unified region to

pub(super?) struct UnifiedRegion { resolved: Option<Region<'tcx>>, } impl UnifiedRegion { /// Something something the resolved region may not be nameable by /// this region variable. pub fn get_resolved_region_ignoring_universes(self) -> Option<Region<'tcx>> { self.resolved } }

lcnr · 2023-03-08T11:19:40Z

@bors r+ rollup=never

bors · 2023-03-08T11:19:42Z

📌 Commit 228f408 has been approved by lcnr

It is now in the queue for this repository.

aliemjay · 2023-03-08T11:25:54Z

@lcnr one more commit to review 😅

aliemjay · 2023-03-08T11:39:41Z

I'll pull it to another PR. It's not that important.

In case a variable is unified with two universal regions from different universes, use the one with the lower universe as it has a higher chance of being compatible with the variable.

lcnr · 2023-03-08T12:31:55Z

@bors r-

@bors r+

bors · 2023-03-08T12:31:59Z

📌 Commit 0b232d0 has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-03-08T12:33:25Z

⌛ Testing commit 0b232d0 with merge 7c306f6...

bors · 2023-03-08T15:23:10Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 7c306f6 to master...

rust-timer · 2023-03-08T16:43:13Z

Finished benchmarking commit (7c306f6): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	2
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	3
Improvements ✅ (secondary)	-1.4%	[-1.7%, -0.7%]	9
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	3

Max RSS (memory usage)

Results

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)	1.7%	[1.1%, 2.2%]	2
Improvements ✅ (primary)	-3.3%	[-3.3%, -3.2%]	2
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	-3.3%	[-3.3%, -3.2%]	2

Cycles

Results

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%	[-3.1%, -2.6%]	7
All ❌✅ (primary)	-	-	0

nnethercote · 2023-03-08T20:57:57Z

The improvements greatly outweight the miniscule regressions here. The cycles and wall-time numbers for wg-grammar are especially nice.

@rustbot label: +perf-regression-triaged

rustbot assigned lcnr Feb 16, 2023

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 Feb 16, 2023

aliemjay mentioned this pull request Feb 16, 2023

remove obsolete givens from regionck #107376

Merged

aliemjay added 4 commits March 5, 2023 12:00

resolve to universal regions when possible

f3dfa52

s/unification_table/unification_table_mut

79dca7b

Give a more clear name.

oops! new unsoundness

eea5604

Bless tests and show an introduced unsoundness related to exits<'a> { forall<'b> { 'a == 'b } }. We now resolve the var ?a in U0 to the placeholder !b in U1.

fix the new unsoundness

bfd3501

aliemjay force-pushed the resolve-var-region branch from 8eee7db to bfd3501 Compare March 5, 2023 09:50

bless rustdoc tests

095b5fa

aliemjay marked this pull request as ready for review March 5, 2023 11:42

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 Mar 6, 2023

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 Mar 6, 2023

aliemjay mentioned this pull request Mar 7, 2023

[PERF] allow region unification to change region universes #108867

Closed

lcnr approved these changes Mar 8, 2023

View reviewed changes

address review comment

228f408

aliemjay force-pushed the resolve-var-region branch from 182ae54 to 228f408 Compare March 8, 2023 11:07

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 Mar 8, 2023

aliemjay mentioned this pull request Mar 8, 2023

prefer universal region from lower universe in region unification table #108899

Closed

aliemjay force-pushed the resolve-var-region branch from d9dec12 to 228f408 Compare March 8, 2023 11:44

prefer universal from lower universe

0b232d0

In case a variable is unified with two universal regions from different universes, use the one with the lower universe as it has a higher chance of being compatible with the variable.

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2023

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2023

bors merged commit 7c306f6 into rust-lang:master Mar 8, 2023

rustbot added this to the 1.70.0 milestone Mar 8, 2023

rustbot added the perf-regression Performance regression. label Mar 8, 2023

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 8, 2023

aliemjay deleted the resolve-var-region branch March 9, 2023 06:35

lcnr mentioned this pull request Feb 22, 2024

region unification: update universe of region vars #121442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always resolve to universal regions if possible #108121

always resolve to universal regions if possible #108121

aliemjay commented Feb 16, 2023

bors commented Feb 16, 2023

lcnr commented Feb 17, 2023

aliemjay commented Mar 5, 2023

lcnr commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 6, 2023

aliemjay commented Mar 6, 2023

aliemjay commented Mar 7, 2023

lcnr left a comment

lcnr Mar 8, 2023

lcnr commented Mar 8, 2023

bors commented Mar 8, 2023

aliemjay commented Mar 8, 2023

aliemjay commented Mar 8, 2023

lcnr commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

rust-timer commented Mar 8, 2023

nnethercote commented Mar 8, 2023

always resolve to universal regions if possible #108121

always resolve to universal regions if possible #108121

Conversation

aliemjay commented Feb 16, 2023

bors commented Feb 16, 2023

lcnr commented Feb 17, 2023

aliemjay commented Mar 5, 2023

lcnr commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 6, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

aliemjay commented Mar 6, 2023

aliemjay commented Mar 7, 2023

lcnr left a comment

Choose a reason for hiding this comment

lcnr Mar 8, 2023

Choose a reason for hiding this comment

lcnr commented Mar 8, 2023

bors commented Mar 8, 2023

aliemjay commented Mar 8, 2023

aliemjay commented Mar 8, 2023

lcnr commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

rust-timer commented Mar 8, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

nnethercote commented Mar 8, 2023