Start using pattern types in libcore

[oli-obk]

View all comments

cc #135996

Replaces the innards of NonNull with *const T is !null.

This does affect LLVM's optimizations, as now reading the field preserves the metadata that the field is not null, and transmuting to another type (e.g. just a raw pointer), will also preserve that information for optimizations. This can cause LLVM opts to do more work, but it's not guaranteed to produce better machine code.

Once we also remove all uses of rustc_layout_scalar_range_start from rustc itself, we can remove the support for that attribute entirely and handle all such needs via pattern types

[compiler-errors]

Well r=me when/if it does get unblocked.

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[Veykril]

Re rust-analyzer blocker, as it turns out to implement pattern types (to a degree that would unblock this) it comes down to the same architecture changes required as for rust-lang/rust-analyzer#7434 (which I am currently looking into), so that will take a bit.

[rustbot]

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[oli-obk]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 339a92e (339a92ed90cab1177510feacee981e87fa5f004c, parent: 8d50bccc5bd70be9f5a2fc98c0857e24b3dcdf85)

[oli-obk]

Oh no the changes from #152702 had subtle effects on what LLVM sees when pattern types are used inside NonNull, so now some opts don't work anymore

[rust-timer]

Finished benchmarking commit (339a92e): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 0.6%]	8
Regressions ❌ (secondary)	1.1%	[0.3%, 2.5%]	18
Improvements ✅ (primary)	-0.8%	[-3.4%, -0.1%]	91
Improvements ✅ (secondary)	-0.6%	[-10.0%, -0.0%]	87
All ❌✅ (primary)	-0.7%	[-3.4%, 0.6%]	99

Max RSS (memory usage)

Results (primary 0.8%, secondary -0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	5.6%	[3.1%, 8.5%]	3
Regressions ❌ (secondary)	4.5%	[1.8%, 6.4%]	3
Improvements ✅ (primary)	-4.0%	[-4.5%, -3.6%]	3
Improvements ✅ (secondary)	-3.7%	[-7.5%, -0.9%]	4
All ❌✅ (primary)	0.8%	[-4.5%, 8.5%]	6

Cycles

Results (primary -1.8%, secondary -2.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.7%	[1.7%, 1.7%]	1
Regressions ❌ (secondary)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-2.2%	[-2.5%, -1.9%]	9
Improvements ✅ (secondary)	-3.4%	[-9.4%, -1.2%]	12
All ❌✅ (primary)	-1.8%	[-2.5%, 1.7%]	10

Binary size

Results (primary -0.1%, secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.2%]	9
Regressions ❌ (secondary)	0.2%	[0.0%, 0.5%]	4
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	11
Improvements ✅ (secondary)	-1.6%	[-3.1%, -0.1%]	2
All ❌✅ (primary)	-0.1%	[-0.3%, 0.2%]	20

Bootstrap: 479.657s -> 474.169s (-1.14%)
Artifact size: 396.97 MiB -> 393.93 MiB (-0.76%)

[oli-obk]

Ok that is some nice perf, but is it just costing runtime perf?

[oli-obk]

Oh no the changes from #152702 had subtle effects on what LLVM sees when pattern types are used inside NonNull, so now some opts don't work anymore

basically this PR doesn't change the MIR of

pub fn zero(d: &mut [Vec<i32>]) {
    let n = d.len();
    for i in 0..n {
        assert!(d[i].len() == n);
        for j in 0..n {
            d[i][j] = 0;
        }
    }
}

at all.

The MIR of it contains no function calls, it's fully inlined. So the only thing that can cause a regression here is the fact that somewhere we transmute across pattern type and LLVM chokes on that, even tho the pattern type never shows up in MIR.

[oli-obk]

Oh no the changes from #152702 had subtle effects on what LLVM sees when pattern types are used inside NonNull, so now some opts don't work anymore

nope that's not it. I reverted all of it and it didn't affect anything.

[oli-obk]

The main change of the zero function in LLVM IR is

-define void @zero(ptr noalias noundef nonnull align 8 %d.0, i64 noundef range(i64 0, 384307168202282326) %d.1) unnamed_addr #0 {
+define void @zero(ptr noundef nonnull align 8 %d.0, i64 noundef range(i64 0, 384307168202282326) %d.1) unnamed_addr #0 {

so the loss of the noalias attribute. Maybe that can cause the later opts to change.

There are no opt changes until SpeculativeExecutionPass

-  %_28 = icmp ult i64 %iter1.sroa.0.0, %_6
+  %_26 = load i64, ptr %1, align 8, !noundef !2
+  %_28 = icmp ult i64 %iter1.sroa.0.0, %_26
+  br i1 %_28, label %bb10, label %panic2

While I don't know much about llvm opts, I find it reasonable that the lack of noalias causes re-reads of the (nested) slice's length, as it could theoretically be changed elsewhere.
something like that waves hands

[oli-obk]

@rustbot ready

[oli-obk]

This caused the bug where noalias was not set anymore if a pattern type was behind a &mut ref

[oli-obk]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 72aec09 (72aec09742d9339443f6e63421b7babc09c33c3d, parent: ea5573a6c6e5e932f917ec4a8e6d8efdeb9f394d)

[rust-timer]

Finished benchmarking commit (72aec09): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary 2.4%, secondary 5.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.4%	[1.7%, 2.9%]	3
Regressions ❌ (secondary)	5.8%	[5.8%, 5.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[1.7%, 2.9%]	3

Cycles

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

Binary size

Results (primary 0.1%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 480.191s -> 478.416s (-0.37%)
Artifact size: 395.10 MiB -> 395.20 MiB (0.03%)

[oli-obk]

The entire regression is in rustdoc lowering (clean). Unsure why, but it's also very tiny. Likely a more structural issue. Rustdoc clean should not end up seeing any pattern types

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[oli-obk]

cc @BoxyUwU