ensure llvm gets bounds for size_of_val on slices with an element size of one

[asquared31415]

View all comments

fixes #159754

Also simplifies the zero size element case to return 0 without a multiplication.

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: codegen, compiler
• codegen, compiler expanded to 74 candidates
• Random selection from 17 candidates

[hanna-kruppe]

Let's see if perf gives us any evidence that this assume is useful in real code: @bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3831952 (38319520271018776c07fa3cb7a1968aaeca1bf4)
Base parent: 008fa22 (008fa22ce3f8d3c8dfaca2e6486043c2b21851eb)

[rust-timer]

Finished benchmarking commit (3831952): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.4%, 0.6%]	2

Max RSS (memory usage)

Results (primary 2.5%, secondary -2.0%)

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

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

Cycles

Results (secondary 2.4%)

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

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

Binary size

Results (secondary 0.0%)

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

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

Bootstrap: 489.346s -> 487.704s (-0.34%)
Artifact size: 387.69 MiB -> 387.63 MiB (-0.01%)

[hanna-kruppe]

Interesting, the regression looks like the assume does unlock some more optimizations -- slightly more time spent in LLVM and slight decrease in linked_artifact and object_file sizes.

Although I'm a bit confused why, from poking at the generated LLVM IR. The only mentions of size_of_val_raw I could find were in debug info and seem to refer to trait objects. There are also occurrences of mul nuw nsw %x, 1 but at a glance most looked like &[T] or Box<[T]> related functions that already have range metadata on the length.

[hanna-kruppe]

Oh, the implementation here is shared with size_of_val. So with this PR we emit IR like this:

; test_size_val::s
; Function Attrs: noinline nonlazybind uwtable
define noundef i64 @_RNvCslj8j6VNIQG1_13test_size_val1s(ptr noalias nofree noundef nonnull readonly captures(address, read_provenance) %x.0, i64 noundef range(i64 0, -9223372036854775808) %x.1) unnamed_addr #0 {
start:
  %0 = icmp ule i64 %x.1, 9223372036854775807
  call void @llvm.assume(i1 %0)
  ret i64 %x.1
}

Or, at opt-level=0:

; core::mem::size_of_val::<[u8]>
; Function Attrs: inlinehint nonlazybind uwtable
define i64 @_RINvNtCs3cghN7566d9_4core3mem11size_of_valShECslj8j6VNIQG1_13test_size_val(ptr %val.0, i64 %val.1) unnamed_addr #0 {
start:
  %0 = icmp ule i64 %val.1, 9223372036854775807
  ret i64 %val.1
}

That's silly. We should not add assumes to code paths that already have the same information via range metadata.

[asquared31415]

What code reproduces that case?

[hanna-kruppe]

#[inline(never)]
pub fn s(x: &[u8]) -> usize {
    size_of_val(x)
}

[asquared31415]

It appears that the range information is several calls up the call stack, in that it's only on s, and by the size_of_val intrinsic, it's completely lost because the types are coerced to raw pointers. I'm unsure how I'd implement checking that the type is &[T] or that a value has the range information already.

[hanna-kruppe]

Hypothetically, you could split the existing intrinsic into two, mirroring the stable {size,align}_of_val vs {size,align}_of_val_raw distinction. But I think that creating more intrinsics would require stronger evidence that there's real world optimization potential to be had here (and it can't be gained by other means) than we've seen so far. So, thanks for taking a stab at this, but at present I'd suggest that introducing assumes to fix #159754 (at least in its current state) is indeed not worth it.

[asquared31415]

I expect that the only major perf impact would be if more precise range info helps with proving layout calculations cannot overflow. Something like size_of::<Header>() + size_of_val_raw(tail_ptr). Potentially not worth the compile time increase, if that is caused by giving LLVM more things to do, like the perf run suggests.

I understand this question is probably only Vibes Based, but do you believe that there would be an appreciable improvement if we did somehow omit the assume for cases where the caller already had the appropriate information? I would expect that there's a lot of &[u8] going on in the compiler that could very well be the culprit, but may already have the range information.

[asquared31415]

Oh that's fun, apparently the length field of &Dst for a user defined ADT doesn't get the range metadata like a slice reference does.

[hanna-kruppe]

Oh that's fun, apparently the length field of &Dst for a user defined ADT doesn't get the range metadata like a slice reference does.

That sounds like part of #152788, cc @scottmcm maybe you have input for this PR and/or the original issue #159754

---

I expect that the only major perf impact would be if more precise range info helps with proving layout calculations cannot overflow. Something like size_of::<Header>() + size_of_val_raw(tail_ptr). Potentially not worth the compile time increase, if that is caused by giving LLVM more things to do, like the perf run suggests.

I think for head + tail calculations the equally or more important part (which we're currently missing) is no-wrap flags on the addition.

I understand this question is probably only Vibes Based, but do you believe that there would be an appreciable improvement if we did somehow omit the assume for cases where the caller already had the appropriate information? I would expect that there's a lot of &[u8] going on in the compiler that could very well be the culprit, but may already have the range information.

At this point my main concern is why would we even invest any more effort into this when there's no clear evidence of it helping. The original issue doesn't have any, rust-perf is a mixed bag but mostly neutral, and the one benchmark that improved may be due to other reasons than the assumes being actually useful, e.g.:

• If we don't have range metadata for the length of &Foo<[u8]> then we should add that, not add assumes.
• There's cases where we're missing out on range metadata (e.g., The NonZero types don't tell LLVM that they're non-zero on get #49572) even on types where we usually have them. The assumes added here may paper over some such issues, but fixing the range metadata would be better.
• While not very likely, it's always possible that a locally-negative impact of an assume (e.g., blocking a peephole optimization due to there being more than one use of the length) can butterfly effect into overall better optimization results.

I don't want to just go and close this PR, because then you can't easily reopen it if you find a reviewer who's more optimistic about this. But I would encourage you to find other issues to contribute to which are less of a tar pit and have a clearer value proposition.

[asquared31415]

So, for fun, I looked into putting range metadata on lengths of &Dst earlier, and it makes LLVM very unhappy and regresses codegen, and probably doesn't even help that much. It is sufficient for LLVM to figure out the range of the output values, so maybe if it gets inlined enough, the bad codegen (which ends up using a whole extra register) goes away? Not very hopeful though.

Agreed that it's probably not worth pursuing range info even on the most simple cases though, since it's not showing any significant improvements, has minor regressions, and complicates the compiler code. I even looked into doing the two intrinsics approach, and it potentially might fix the regression, but it wouldn't necessarily gain anything. And of course it makes the compiler code even more complicated.

[hanna-kruppe]

Thank you for looking into this, negative results are still results :)

[scottmcm]

I think this probably doesn't show up in perf because #148350 means that &[T] does have metadata (specifically range parameter attribute when the slice is a parameter), which is the majority case. I don't think people using *const [_] is nearly as common.

[scottmcm]

• If we don't have range metadata for the length of &Foo<[u8]> then we should add that

Yeah, really the core problem is that there's no valid_range restriction on the length field on reference-to-slice: https://rust.godbolt.org/z/n8P7osbo9 (b: u64 is ..)

That boils down to the same problem as layout-based niches in references, though, which is hard.

[asquared31415]

For Reasons(tm) that data gets added in backends, later after layout calculations. &[u16] does get a range of (0, isize::MAX/2) in that later step. That step however, only applies for ty::Ref(ty::Slice(T)) shaped things though. Modifying it to work for other types isn't too hard, but it certainly feels like adding more hacks to what's already a hack.

[scottmcm]

I think for head + tail calculations the equally or more important part (which we're currently missing) is no-wrap flags on the addition.

If someone wants to pick up #152843 and push it over the line, that should help for some of it. It looks like it got stalled out.

[scottmcm]

For Reasons(tm) that data gets added in backends, later after layout calculations.

Yup, I wrote the PR that hacked it in there :P