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

64bit indexing fused adam #5187

Merged
merged 11 commits into from
Apr 22, 2024
Merged

Conversation

garrett4wade
Copy link
Contributor

The Issue

Applying FusedAdam on large tensors will cause an error CUDA error: an illegal memory access was encountered.

#3429

NVIDIA/apex#1654

PR Content

Following the solution in the apex repository (NVIDIA/apex#1765), changing indexing type to int64 if necessary.

@garrett4wade
Copy link
Contributor Author

@garrett4wade please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@@ -30,7 +30,7 @@ typedef enum : int {

using MATH_T = float;

template <typename T>
template <typename T, typename index_t>
struct AdamFunctor {
__device__ __forceinline__ void operator()(int chunk_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this chunk_size is still an int and not int64/index_t

Copy link
Contributor

Choose a reason for hiding this comment

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

@garrett4wade - thoughts on this?

@loadams
Copy link
Contributor

loadams commented Apr 9, 2024

FYI @garrett4wade - thoughts on the comments above or merging this PR?

@loadams loadams self-assigned this Apr 9, 2024
@loadams loadams added this pull request to the merge queue Apr 22, 2024
Merged via the queue into microsoft:master with commit 9b6ef9e Apr 22, 2024
12 checks passed
rraminen pushed a commit to ROCm/DeepSpeed that referenced this pull request May 9, 2024
## The Issue

Applying `FusedAdam` on large tensors will cause an error `CUDA error:
an illegal memory access was encountered`.

microsoft#3429

NVIDIA/apex#1654

## PR Content

Following the solution in the apex repository
(NVIDIA/apex#1765), changing indexing type to
`int64` if necessary.

---------

Co-authored-by: Michael Wyatt <michaelwyatt@microsoft.com>
Co-authored-by: Logan Adams <114770087+loadams@users.noreply.github.com>
umchand pushed a commit to umchand/DeepSpeed that referenced this pull request May 20, 2024
## The Issue

Applying `FusedAdam` on large tensors will cause an error `CUDA error:
an illegal memory access was encountered`.

microsoft#3429

NVIDIA/apex#1654

## PR Content

Following the solution in the apex repository
(NVIDIA/apex#1765), changing indexing type to
`int64` if necessary.

---------

Co-authored-by: Michael Wyatt <michaelwyatt@microsoft.com>
Co-authored-by: Logan Adams <114770087+loadams@users.noreply.github.com>
dbyoung18 pushed a commit to dbyoung18/DeepSpeed that referenced this pull request Jun 11, 2024
## The Issue

Applying `FusedAdam` on large tensors will cause an error `CUDA error:
an illegal memory access was encountered`.

microsoft#3429

NVIDIA/apex#1654

## PR Content

Following the solution in the apex repository
(NVIDIA/apex#1765), changing indexing type to
`int64` if necessary.

---------

Co-authored-by: Michael Wyatt <michaelwyatt@microsoft.com>
Co-authored-by: Logan Adams <114770087+loadams@users.noreply.github.com>
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.

3 participants