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

[JIT] Ensure offset is a multiple of 4 to fix "Philox" RNG in jitted kernels #50169

Closed

Conversation

mcarilli
Copy link
Collaborator

@mcarilli mcarilli commented Jan 6, 2021

Immediately-upstreamable part of #50148.

This PR fixes what I'm fairly sure is a subtle bug with custom Philox class usage in jitted kernels. Philox constructors in kernels take the cuda rng generator's current offset. The Philox constructor then carries out offset/4 (a uint64_t division) to compute its internal offset in its virtual Philox bitstream of 128-bit chunks. In other words, it assumes the incoming offset is a multiple of 4. But (in current code) that's not guaranteed. For example, the increments used by these eager kernels could easily make offset not divisible by 4.

I figured the easiest fix was to round all incoming increments up to the nearest multiple of 4 in CUDAGeneratorImpl itself.

Another option would be to round the current offset up to the next multiple of 4 at the jit point of use. But that would be a jit-specific offset jump, so jit rng kernels wouldn't have a prayer of being bitwise accurate with eager rng kernels that used non-multiple-of-4 offsets. Restricting the offset to multiples of 4 for everyone at least gives jit rng the chance to match eager rng. (Of course, there are still many other ways the numerics could diverge, like if a jit kernel launches a different number of threads than an eager kernel, or assigns threads to data elements differently.)

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 6, 2021

💊 CI failures summary and remediations

As of commit 25cdf69 (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 6 times.

@mcarilli mcarilli requested a review from ngimel January 6, 2021 23:02
@H-Huang H-Huang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 7, 2021
@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #50169 (25cdf69) into master (eef5eb0) will increase coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #50169      +/-   ##
==========================================
+ Coverage   80.49%   80.68%   +0.18%     
==========================================
  Files        1900     1900              
  Lines      206254   206254              
==========================================
+ Hits       166018   166409     +391     
+ Misses      40236    39845     -391     

@ngimel
Copy link
Collaborator

ngimel commented Jan 8, 2021

Please add the PR description as a note somewhere in the code.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ngimel merged this pull request in 271240a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants