Repository navigation
Conversation
Author
|
Hi @Tcc0403, could you take a look when you get a chance? This fixes #1570. The experimental embedding kernels computed row offsets in int32, so they hit an illegal memory access once the output or the table went past 2^31 elements. @xiaoqiangheye this should cover the case you reported, if you want to try it. |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes #1570.
The experimental embedding kernels compute row offsets in int32. The forward pass multiplies
offsets_m * embedding_dimandindices * embedding_dim, and the backward pass does the same. Once the output or the embedding table holds 2^31 or more elements, those products wrap around. The kernel then reads and writes out of bounds and crashes with an illegal memory access.The fix promotes
pid_mto int64 before computingstart_m. It also casts the loaded indices to int64. That keeps every offset product in int64, and it works even when the caller passes int32 indices. The change is the same inembedding_forward_kernelandembedding_backward_kernel.Details
I added two tests. Each one crosses the int32 boundary on a different side.
test_embedding_output_offsets_beyond_int32makesnum_tokens * embedding_dimlarger than 2^31. It checks the output rows past the boundary and also checks the gradient counts.test_embedding_table_offsets_beyond_int32_with_int32_indicesmakes the table larger than 2^31 elements and reads the last rows with int32 indices. Then it checks the output and the gradient.The existing
test_embedding_correctnessis skip-marked as under experimentation. So the new tests callLigerEmbeddingFunctiondirectly. They skip when the GPU has less than 24 GiB free, so they won't break smaller CI runners.On GB10 (DGX Spark) the CPU and GPU share memory.
torch.cuda.mem_get_info()reports only about 1 GiB free there because it doesn't count reclaimable page cache. So these tests skip on that machine by default. I disabled the skip check locally to run them.Testing Done
Without the fix, both new tests fail with
CUDA error: an illegal memory access was encountered. With the fix, both pass.make testto ensure correctness (ran the embedding tests above)make checkstyleto ensure code stylemake test-convergenceto ensure convergence (N/A, the experimental embedding isn't used in the convergence tests)