Skip to content

Conversation

@sayantn
Copy link
Contributor

@sayantn sayantn commented Nov 10, 2025

Retrying #1928, this time actually checking for overflow. LLVM sees through this, and eliminates the branch/selects https://godbolt.org/z/WsYvfjPas

cc @RalfJung this also removes uses of a few intrinsics from avx2

@rustbot
Copy link
Collaborator

rustbot commented Nov 10, 2025

r? @folkertdev

rustbot has assigned @folkertdev.
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

@eduardosm
Copy link
Contributor

simd_shl and simd_shr are documented to be UB on overflow, but the implementations of this PR will unconditionally call these intrinsics.

@sayantn
Copy link
Contributor Author

sayantn commented Nov 11, 2025

Ah, so I need to add a simd_and to make it safe. Thanks for pointing it out

@sayantn
Copy link
Contributor Author

sayantn commented Nov 12, 2025

It looks like adding that one simd_and messes up the whole LLVM optimization. Strictly speaking, this is fine as long as we are only in LLVM backend, LLVM generates poison values if the shift value overflows and we are filtering them out in the next step. But obviously that's not a good argument, as the main motive for this change is to make it more portable across backends. I will look into what needs to be done

edit: https://godbolt.org/z/99zrnb5Gj

edit2: somehow using simd_select(good, count, T::splat(dummy_value)) works. It lets LLVM collapse the 2 selects, while also keeping the simd_shl call safe from Rust (https://godbolt.org/z/cc4EKfeb7)

@sayantn
Copy link
Contributor Author

sayantn commented Nov 12, 2025

https://godbolt.org/z/cafKWsf6x I think these should do the job, @eduardosm could you check please?

edit: seems to pass miri on overflow playground

@sayantn sayantn force-pushed the vector-shifts branch 2 times, most recently from 67db9b3 to a299d29 Compare November 12, 2025 02:19
@eduardosm
Copy link
Contributor

https://godbolt.org/z/cafKWsf6x I think these should do the job, @eduardosm could you check please?

That looks good to me 👍🏻

@sayantn
Copy link
Contributor Author

sayantn commented Nov 13, 2025

Thanks @eduardosm :love:, then it should be good to merge

@folkertdev folkertdev added this pull request to the merge queue Nov 13, 2025
Merged via the queue into rust-lang:master with commit 738a603 Nov 13, 2025
73 checks passed
@sayantn sayantn deleted the vector-shifts branch November 13, 2025 11:47
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