Hint the maximum length permitted by invariant of slices

[197g]

One of the safety invariants of references, and in particular of references to slices, is that they may not cover more than isize::MAX bytes. The unsafe from_raw_parts constructors of slices explicitly requires the caller to guarantee this fact. Violating it would also be UB with regards to the semantics of generated llvm code.

This effectively bounds the length of a (non-ZST) slice from above by a compile time constant. But when the length is loaded from a function argument it appears llvm is not aware of this requirement. The additional value range assertions allow some further elision of code branches, including overflow checks, especially in the presence of artithmetic on the indices.

This may have a performance impact, adding more code to a common method but allowing more optimization. I'm not quite sure, is the Rust side of const-prop strong enough to elide the irrelevant match branches?

Fixes: #67186

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[tesuji]

Could you merge the allow_internal_unstable attribute on len method like below?

#[allow_internal_unstable(const_fn_union,const_unreachable_unchecked)]

I guess the attr doesn't support auto-merging multiple invocations of it.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[197g]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[197g]

That should do all the implementation work. Would you mind to start a perf run to see if this adversly influences inlining somewhere?

[Mark-Simulacrum]

@bors try @rust-timer queue

This seems like it's going to complicate the codegen for len() -- adding an assume or similar construct -- which might have a pretty high compile time cost even if not so high a runtime cost.

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit b885434b728adc2bcce88480ed755fb1e4ecb837 with merge 8612477075acc90b2716af836a922f8965189225...

[rust-log-analyzer]

The job dist-x86_64-linux of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

 Compiling std v0.0.0 (/checkout/library/std)
Compiling compiler_builtins v0.1.35
Compiling unwind v0.0.0 (/checkout/library/unwind)
Compiling profiler_builtins v0.0.0 (/checkout/library/profiler_builtins)
error[E0636]: the feature `const_fn` has already been declared
|
85 | #![feature(const_fn)]
| ^^^^^^^^
---
To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace profiler compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host x86_64-unknown-linux-gnu --target x86_64-unknown-linux-gnu
Build completed unsuccessfully in 0:01:18
== clock drift check ==
local time: Thu Sep 24 14:24:23 UTC 2020
local time: Thu Sep 24 14:24:23 UTC 2020
network time: Thu, 24 Sep 2020 14:24:23 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (4743) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[bors]

💔 Test failed - checks-actions

[197g]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Looks like the feature const_fn got added from somewhere else, too. Should be fixed.

[Mark-Simulacrum]

@bors try

[bors]

⌛ Trying commit 482f306df072d2b20507b5577c780b8dfa477edc with merge 9c1568be513095701cd7735f2d9a1b5f1fbac104...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9c1568be513095701cd7735f2d9a1b5f1fbac104 (9c1568be513095701cd7735f2d9a1b5f1fbac104)

[rust-timer]

Queued 9c1568be513095701cd7735f2d9a1b5f1fbac104 with parent e599b53, future comparison URL.

[Mark-Simulacrum]

I don't know what to do about CI failure either. I presume this is a limitation of the beta compiler? Can you try putting #[cfg(bootstrap)] on the assume?

Please also keep commits squashed as you edit (both this change and further ones if they become necessary); this PR has a small line diff which should not need more than one commit.

[197g]

That seems to have done it 🎉 I've squashed the largest part of the history but kept this at two commits, one introducing the test and the other the actual change. I hope that's okay.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[Mark-Simulacrum]

I would prefer to have a single commit, makes it easier to find the code introduced alongside the test. r=me with it squashed.

[197g]

Okay, that's fine for me.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[Mark-Simulacrum]

@bors r+ rollup=never

[bors]

📌 Commit e44784b has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit e44784b with merge beb5ae4...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing beb5ae4 to master...

[tesuji]

Note that this PR caused a slight regression on max-rss; https://perf.rust-lang.org/compare.html?start=4ccf5f731bb71db3470002d6baf5ab4792b821d9&end=beb5ae474d2835962ebdf7416bd1c9ad864fe101&stat=max-rss

I guess LLVM hasn't changed much since: #54995 (comment)

[ecstatic-morse]

Final perf results are in. This seemed to cause a slowdown across the board with the exception of some synthetic benchmarks. Except for the aforementioned stress test, check builds are not improved or a bit slower.

The case in the mir-opt test doesn't seem like it would occur very often in real code, and LLVM seems to explicitly recommend against doing this, so I think it's pretty clear we should revert this.

[ecstatic-morse]

@lzutao We have an automated script for detecting these kinds of regressions and run it weekly as part of perf-triage. That usually happens on a Monday. Also, there's no max-rss regression here: things were flat this week. max-rss is too unstable to make inferences about with only two data points.