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

Cuda GatherCompiler fails on low dimensionality (failing test) #71

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

swfsql
Copy link
Contributor

@swfsql swfsql commented Jun 22, 2024

Related to #70.

This showcases a failure for the Cuda GatherCompiler compilation pass.

To fetch the PR and run the tests:

git fetch origin pull/71/head:cuda_gather_failure
git switch cuda_gather_failure
cd crates/luminal_cuda
cargo test test_gather_compiler

The test_gather_compiler_r1 passes:
https://github.com/jafioti/luminal/pull/71/files#diff-ead6d80249befe24da4ea23c236763f82cc1eccfccbb92035217e620e8bf5184R434-R459

While test_gather_compiler_r0 fails:
https://github.com/jafioti/luminal/pull/71/files#diff-ead6d80249befe24da4ea23c236763f82cc1eccfccbb92035217e620e8bf5184R409-R432

The failure error is:

---- binary::tests::test_gather_compiler_r0 stdout ----
thread 'binary::tests::test_gather_compiler_r0' panicked at src/binary.rs:379:46:
index out of bounds: the len is 2 but the index is 2

The relevant line of code is:

let embed_dim = emb_shape.shape()[2].to_usize().unwrap();

I'm not sure how cuda relates to those operations, but the first thought would be to maybe take the last dimension value instead of the third (emb_shape.shape()[2]) one?

@jafioti jafioti marked this pull request as ready for review June 22, 2024 23:09
@jafioti
Copy link
Owner

jafioti commented Jun 22, 2024

Yeah you're right, taking the last dimension here was the correct solution. Can you just change that line to

let embed_dim = emb_shape.shape().last().unwrap().to_usize().unwrap();

That should get the test passing, and then I'll merge

@swfsql
Copy link
Contributor Author

swfsql commented Jun 25, 2024

Thanks, I've added this change, and the tests pass.

@jafioti jafioti merged commit 7136f54 into jafioti:main Jul 3, 2024
3 checks passed
@jafioti
Copy link
Owner

jafioti commented Jul 3, 2024

Great thanks!

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.

2 participants