Skip to content
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

Saturating casts for integers, like _mm_packus_epi16 and _mm_packs_epi16 #369

Open
okaneco opened this issue Oct 6, 2023 · 5 comments
Open
Labels
C-feature-request Category: a feature request, i.e. not implemented / a PR

Comments

@okaneco
Copy link
Contributor

okaneco commented Oct 6, 2023

My most common use case is for saturating narrowing casts, such as from i16 to u8. It would be great to take advantage of native instructions without first having to use simd_clamp. saturating_cast would reduce code and chances of errors from clamping with incorrect values.

I'm not sure about the availability of this feature on other architectures. It might make sense to split this for narrowing and widening features based on availability. I'm not familiar with other architectures, but it looks like ARM has narrowing casts with VQMOVN/VQMOVUN.

Perhaps there could be a fallback which does simd_clamp().cast() (or other optimal construction) for architectures that don't have these instructions.

@okaneco okaneco added the C-feature-request Category: a feature request, i.e. not implemented / a PR label Oct 6, 2023
@calebzulawski
Copy link
Member

Is there a saturating cast for regular (scalar) integers? Not that that's a requirement, but it's a good guide as to what to include. I'm not opposed to adding it, though. It's worth noting that simd_clamp().cast() does the right thing on both x86-64 and aarch64: godbolt

@okaneco
Copy link
Contributor Author

okaneco commented Oct 7, 2023

Not that I'm aware of in std but I wish scalars had this, as I need it a lot in contexts like image processing.

Part of what motivated this issue was that I was using i16x4/u8x4 on x86-64 with the base feature profile. I noticed that it didn't produce similar code to the i16x8 example without some coaxing (which seems to pessimize the aarch output).
https://rust.godbolt.org/z/edxro99e3

@calebzulawski
Copy link
Member

calebzulawski commented Oct 7, 2023

That's definitely an LLVM issue, I'm not sure why it only happens with the 4-length vectors. I opened the linked issue to track this. I reviewed the LLVM source and simd_clamp is the right way to indicate saturation:

/// Detect patterns of truncation with unsigned saturation:
///
/// 1. (truncate (umin (x, unsigned_max_of_dest_type)) to dest_type).
///   Return the source value x to be truncated or SDValue() if the pattern was
///   not matched.
///
/// 2. (truncate (smin (smax (x, C1), C2)) to dest_type),
///   where C1 >= 0 and C2 is unsigned max of destination type.
///
///    (truncate (smax (smin (x, C2), C1)) to dest_type)
///   where C1 >= 0, C2 is unsigned max of destination type and C1 <= C2.
///
///   These two patterns are equivalent to:
///   (truncate (umin (smax(x, C1), unsigned_max_of_dest_type)) to dest_type)
///   So return the smax(x, C1) value to be truncated or SDValue() if the
///   pattern was not matched.
static SDValue detectUSatPattern(SDValue In, EVT VT, SelectionDAG &DAG,
                                 const SDLoc &DL) {

@okaneco
Copy link
Contributor Author

okaneco commented Oct 8, 2023

Thanks for that.
It's good to know that what I saw was an exception with the extra instructions.

I've possibly come across another issue with 4-vectors and casting from i16x4 to u8x4. If I use a temporary array and manually unroll the cast into that destination, it produces vector instructions.
https://rust.godbolt.org/z/WrKdjx9sa

Feel free to move this to its own issue if needed.

@okaneco
Copy link
Contributor Author

okaneco commented Mar 1, 2024

Both optimization patterns have now been fixed upstream. The saturating truncating cast fix is in LLVM 18, available on nightly
https://rust.godbolt.org/z/edxro99e3
The truncating cast issue was recently fixed and will probably be in LLVM 19 (above, https://rust.godbolt.org/z/WrKdjx9sa)
https://llvm.godbolt.org/z/hTbnadsnj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: a feature request, i.e. not implemented / a PR
Projects
None yet
Development

No branches or pull requests

2 participants