Improve Span bounds check elimination by adding _assumeNonNegative to count #83172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
eeckstein requested review from a team, artemcm, rjmccall and tshortli as code owners eeckstein force-pushed the span-bounds-checking branch from 02ac763 to 451290d Compare 
rjmccall approved these changes Jul 18, 2025
lib/IRGen/GenBuiltin.cpp Outdated
Contributor
There was a problem hiding this comment.
I'm fine with taking this as is, but we should talk to LLVM about whether they'd prefer we use range metadata or llvm.assume for these going forward, and then make both paths use the one they want.
Contributor Author
There was a problem hiding this comment.
Based on benchmark results, the best is to keep the range-metadata if possible.
stephentyrone approved these changes Jul 18, 2025
Contributor
| @swift-ci test |
stephentyrone added a commit that referenced this pull request Jul 21, 2025
LLVM ought to be able to do this transformation for us, but it currently fails to do so. We can code around it easily enough. #83172 has a better long-term fix.
stephentyrone added a commit to stephentyrone/swift that referenced this pull request Jul 21, 2025
This allows us to eliminate a comparison; swiftlang#83172 will allow the compiler to do it for us instead in the future.
glessard approved these changes Jul 21, 2025
Contributor
| @eeckstein let's roll a revert of the [Mutable]Span changes from #83150 into this for the main branch. |
So far we only supported this for `load` and `call` arguments. Now we use `llvm.assume` for other kind of argument values. This helps optimizing bounds checking for `Span`.
eeckstein force-pushed the span-bounds-checking branch from 451290d to 27dc031 Compare 
Contributor Author
| @swift-ci apple silicon benchmark |
Contributor Author
| @swift-ci test |
stephentyrone added a commit that referenced this pull request Jul 23, 2025
This allows us to eliminate a comparison; #83172 will allow the compiler to do it for us instead in the future. **Explanation:** Changes the implementation of Span's subscript bounds checks to use a single unsigned comparison instead of two signed compares. **Scope:** Narrow. Does not apply to any other API. **Issues:** rdar://156068535 **Original PRs:** #83150 **Risk:** Low. Minor implementation tweaks to API that is not widely adopted. **Testing:** CI **Reviewers:** @glessard @meg-gupta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
rdar://156345209