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

Bug about Dropout CUDA Kernel #68909

Open
MARD1NO opened this issue Nov 25, 2021 · 4 comments
Open

Bug about Dropout CUDA Kernel #68909

MARD1NO opened this issue Nov 25, 2021 · 4 comments
Labels
module: cuda Related to torch.cuda, and CUDA support in general module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@MARD1NO
Copy link

MARD1NO commented Nov 25, 2021

In Dropout.cu (https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/cuda/Dropout.cu#L35)

In 56 lines, you set bool gridxvec_loop_state = 0

When the gridxvec_loop_state is 0, you generate 4 rand numbers。and in other case,you want to use the last 2 rand values:

    if ((VEC == 4) || (gridxvec_loop_state == 0)) {
      rand = curand_uniform4(&state);
    } else {
      // sets up the last two values we generated last iteration to be used this iteration.
      rand.x = rand.z;
      rand.y = rand.w;
      gridxvec_loop_state ^= 1;
    }

Since you set loop_state as 0, it enter the if branch and generate 4 random numbers,you didn't change the loop_state so you never enter the else branch and cannot use the last 2 rand values.

In my opinion, it should be like this:

    if ((VEC == 4) || (gridxvec_loop_state == 0)) {
      rand = curand_uniform4(&state);
      gridxvec_loop_state ^= 1;
    } else {
      // sets up the last two values we generated last iteration to be used this iteration.
      rand.x = rand.z;
      rand.y = rand.w;
      gridxvec_loop_state ^= 1;
    }

cc @albanD @mruberry @jbschlosser @walterddr @ngimel

@eqy
Copy link
Collaborator

eqy commented Nov 29, 2021

CC'ing @mcarilli who might be interested

@soulitzer soulitzer added module: cuda Related to torch.cuda, and CUDA support in general module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Nov 30, 2021
@eqy
Copy link
Collaborator

eqy commented Dec 9, 2021

CC @ngimel

@ngimel
Copy link
Collaborator

ngimel commented Dec 9, 2021

We'll accept a fix for this, but Dropout with vec_size=2 has probably never been used (and even if it is, the bug is not catastrophic, we just don't advance random state enough, but that's fine).

@mcarilli
Copy link
Collaborator

mcarilli commented Dec 9, 2021

I think you're right about the bug and the proposed fix, good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: cuda Related to torch.cuda, and CUDA support in general module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

5 participants