Skip to content

Fix rounding in SSE byte stores - #357

Open
Nathan Chandrasekar (vera-rubin) wants to merge 2 commits into
microsoft:mainfrom
vera-rubin:fix/byte-store-rounding
Open

Nathan Chandrasekar (vera-rubin) wants to merge 2 commits into
microsoft:mainfrom
vera-rubin:fix/byte-store-rounding

Conversation

@vera-rubin

Copy link
Copy Markdown
Contributor

On SSE, storing XMVectorReplicate(0.75f) with XMStoreByte4 or XMStoreUByte4 produces 0x00000001 instead of 0x01010101. XMStoreByteN4 similarly packs 0.25f as 0x1f1f1f1f instead of 0x20202020. The documented Byte4, UByte4, and ByteN4 operations round each component before packing.

We convert each clamped component to an integer before packing with SSE2 instructions, following the short-store implementation.

We checked the reproducer on SSE2, SSE4.1, and AVX2 and added a math3 regression that fails before and passes after, while the full suite retains its five existing math3 failures.

— ChipLab

@walbourn

Copy link
Copy Markdown
Collaborator

I'll need to look closely at these proposed changes. There are a number of issues where I need to ensure that ARM matches, and that it doesn't negatively impact DirectXTex, etc. which is the primary consumer of these functions.

// Mask off any fraction
vResulti = _mm_and_si128(vResulti, MaskUByte4);
// Do a horizontal or of 4 entries
__m128i vResulti2 = _mm_shuffle_epi32(vResulti, _MM_SHUFFLE(3, 2, 3, 2));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we use the same _mm_packs_epi32 / _mm_packs_epi16 pattern with XMStoreUByteN4 and XMStoreByteN4?

@vera-rubin

Copy link
Copy Markdown
Contributor Author

Yes: XMStoreUByteN4 and XMStoreByteN4 now use that pattern (_mm_packus_epi16 for the unsigned one), and XMStoreUByteN4 keeps its truncation, so its output is unchanged. On 27,993 four-lane test vectors, SSE, NEON and scalar now agree for UByteN4, UByte4 and Byte4, after adding the XMVectorRound the NEON XMStoreUByte4 and XMStoreByte4 paths lacked. XMStoreByteN4 still differs (scalar and NEON truncate, SSE rounds as documented): should SSE truncate too, or every path round?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants