Skip to content

Use an explicit unsigned comparison for span index checks #83150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

stephentyrone
Copy link
Contributor

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.

@stephentyrone stephentyrone requested a review from a team as a code owner July 17, 2025 20:51
@stephentyrone
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@meg-gupta meg-gupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the same underlying issue as rdar://154950810 for which @eeckstein has a PR - #82804

This change looks good as well especially if you plan to cherry-pick

Copy link
Contributor

@glessard glessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have the same changes in RawSpan and MutableRawSpan.
I'd already written the bounds checks that way in RawSpan and MutableRawSpan, so no change needed there.

@stephentyrone
Copy link
Contributor Author

stephentyrone commented Jul 17, 2025

@meg-gupta right, hopefully we can land #82804 and fix the problem more generally; this is a stop-gap

@stephentyrone
Copy link
Contributor Author

@swift-ci smoke test

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.
And apply the unsigned-compare optimization one other place that I missed the first time, as revealed by the new tests.
@stephentyrone stephentyrone force-pushed the span-unsigned-comparisons branch from 24e2288 to 1bd4562 Compare July 18, 2025 02:32
@stephentyrone
Copy link
Contributor Author

@swift-ci smoke test

@eeckstein
Copy link
Contributor

LLVM ought to be able to do this transformation for us

LLVM can't do it because it doesn't know that count is >= 0

This seems to be the same underlying issue as rdar://154950810 for which @eeckstein has a PR - #82804

No, it's a different issue.

[...] and fix the problem more generally; this is a stop-gap

Yes, we can. We can add _assumeNonNegative to count:

  public var count: Int { _assumeNonNegative(_count) }

and add a small peephole optimization which takes this constraint into account for converting the two comparisons into a single one.

@eeckstein
Copy link
Contributor

eeckstein commented Jul 18, 2025

and add a small peephole optimization which takes this constraint into account for converting the two comparisons into a single one.

If we pass the information of _assumeNonNegative to LLVM, LLVM can do the work for us: (last two commits of) #83172

@stephentyrone
Copy link
Contributor Author

stephentyrone commented Jul 18, 2025

Looking at the macOS failure here, we're getting a null pointer check that we also absolutely should not have (due to the invariant that the pointer may only be null when count is zero). However, that does not reproduce at desk for me. I assume this is due to the stdlib being built with --swift-stdlib-enable-assertions=true in CI (for macOS only), which is really counter-productive for this sort of testing where we want to ensure that no extra checks are present, even though it's beneficial for other sorts of testing.

The asserts introduce null pointer checks that make the test too fragile.
@stephentyrone stephentyrone force-pushed the span-unsigned-comparisons branch from f731e1d to d0506e4 Compare July 18, 2025 17:08
@stephentyrone
Copy link
Contributor Author

@swift-ci smoke test

@stephentyrone
Copy link
Contributor Author

@swift-ci smoke test macOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants