Optimize offset_from_unsigned result bounds

[SomeFlyingThing]

Hint that result cannot be larger than isize max

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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: compiler
• compiler expanded to 75 candidates
• Random selection from 18 candidates

[jieyouxu]

r? libs

[clarfonthey]

Seems reasonable. For lack of any niche-type APIs that represent this, it should be okay to add this assumption.

@bors r+

[rust-bors]

📌 Commit 7dbde06 has been approved by clarfonthey

It is now in the queue for this repository.

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #161089 (comment)

@bors try jobs=test-various

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#161092), which was unapproved.

View changes since this unapproval

[rust-bors]

💔 Test for f4b1164 failed: CI. Failed job:

• try - test-various (web logs, enhanced plaintext logs)

[clarfonthey]

Not sure if spurious, will have to take a closer look later.

[clarfonthey]

@rustbot review

[clarfonthey]

Finally getting back to this, confused why it passed CI in the PR but not on the rollup, but this is indeed a real issue. Running locally I also have a failure for the test that failed, tests/codegen-llvm/lib-optimizations/append-elements.rs.

So, @rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[nikic]

cc @scottmcm because this seems like a bad idea for all the usual reasons.

[hanna-kruppe]

In addition to all the reasons why adding assume and similar hints is usually a bad idea, I gotta ask @SomeFlyingThing -- did you use an LLM in creating this PR and/or #161071? As pointed out before in #160737 we don't generally accept LLM-assisted PRs without prior approval from reviewers, especially not for soundness-relevant changes.

[clarfonthey]

So, given existing discussion and the breakage, I'm going to close this. Will list the various points separately.

First, per the repo's LLM policy we do at minimum require disclosure of LLM usage, and it appears you have submitted a few PRs post this policy being merged without disclosure despite LLM usage. (Evident by the agent/ branch name.) While this is not a formal moderation warning (I'm not a moderator), I want to make sure you know what the policy is going forward. In general, LLM-assisted PRs require prior approval from reviewers, and we rely on disclosure to enforce this.

Second, we do want to have a higher bar for approval on changes that just blanket-apply assert_unchecked in places, since it is not always a unilateral improvement; while it may increase performance in some places, it may overburden the optimizer in other places and make things worse. I was a bit overzealous in approving this since we didn't fully verify that this was a complete improvement despite the one codegen test.

Third, also just going on the second point, we generally prefer that performance improvements are based in benchmarks or other real data that indicates the improvement, not just guesses on invariants. So, rather than finding invariants that the optimizer cannot assume, we want to show cases where that non-assumption is leading to noticeably worse code.

So, with that said, I'm closing this for now.

[scottmcm]

Also see #123598 where I tried doing this previously -- notably inside the intrinsic which I think is better than in the rust code -- but it still was red in perf.

(The 64-bit only restriction here made me particularly skeptical, too.)