Skip to content

[Common] Added Alignment Requirements for CuBLAS heuristics - #845

Merged
ptrendx merged 5 commits into
NVIDIA:mainfrom
phu0ngng:blas_align
May 22, 2024
Merged

[Common] Added Alignment Requirements for CuBLAS heuristics#845
ptrendx merged 5 commits into
NVIDIA:mainfrom
phu0ngng:blas_align

Conversation

@phu0ngng

@phu0ngngphu0ngng commented May 14, 2024

Copy link
Copy Markdown
Collaborator

Type of change

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have read and followed the contributing guidelines
  • The functionality is complete
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@phu0ngng
added alignment requirements for CuBLAS heuristics
9c5269d
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@phu0ngng
phu0ngng requested a review from ptrendxMay 14, 2024 17:20
@phu0ngng

phu0ngng commented May 14, 2024

Copy link
Copy Markdown
CollaboratorAuthor

/te-ci pytorch

ksivaman
ksivaman approved these changes May 14, 2024
View reviewed changes

@ksivamanksivaman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

ptrendx
ptrendx reviewed May 14, 2024
View reviewed changes
Comment threadtransformer_engine/common/gemm/cublaslt_gemm.cu Outdated
ptrendx
ptrendx reviewed May 14, 2024
View reviewed changes
Comment threadtransformer_engine/common/gemm/cublaslt_gemm.cu Outdated
@ptrendx

ptrendx commented May 14, 2024

Copy link
Copy Markdown
Member

Could we add some test for it?

@deepakn94

deepakn94 commented May 14, 2024

Copy link
Copy Markdown
Contributor

LGTM, thanks. I verified that my failing case now works.

@BtlmdBtlmd mentioned this pull request May 15, 2024
Closed
@phu0ngng
minor rewords
2a56d41
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@ptrendxptrendx added bug Something isn't working 1.7.0 labels May 15, 2024
@phu0ngng
added unit test for gemm with unaligned inputs
e55cd6c
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@phu0ngng

phu0ngng commented May 17, 2024•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Hi, thanks for the feedback. Glad to hear that it works for you now @deepakn94.
@ptrendx I added the unit test to capture this bug. Let me know if there is an issue. Thanks

@phu0ngng
added pytest skip if fp8 is not available
07537f7
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@phu0ngng
changed offset so that it has alignment with 128
1266d44
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@phu0ngng

phu0ngng commented May 17, 2024

Copy link
Copy Markdown
CollaboratorAuthor

/te-ci pytorch

@phu0ngng
phu0ngng requested a review from ptrendxMay 20, 2024 14:15
ptrendx
ptrendx reviewed May 22, 2024
View reviewed changes
Comment threadtransformer_engine/common/gemm/cublaslt_gemm.cu
}
}

uint32_t _getAlignment(uintptr_t address) {

@ptrendxptrendxMay 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: You don't need to start this function with _ - it is already with unnamed namespace and not exposed outside this file.

@ptrendx
ptrendx merged commit 0180163 into NVIDIA:mainMay 22, 2024
ptrendx pushed a commit that referenced this pull request May 22, 2024
@phu0ngng@ptrendx
[Common] Added Alignment Requirements for CuBLAS heuristics (#845)
5895eab
* added alignment requirements for CuBLAS heuristics
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
* minor rewords
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
* added unit test for gemm with unaligned inputs
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
* added pytest skip if fp8 is not available
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
* changed offset so that it has alignment with 128
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
---------
Signed-off-by: Phuong Nguyen <phuonguyen@nvidia.com>
@yaox12yaox12 mentioned this pull request Jul 3, 2024
Closed
@hungryGeek16hungryGeek16 mentioned this pull request May 31, 2026
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ptrendxptrendxptrendx approved these changes
@ksivamanksivamanksivaman approved these changes

Assignees

No one assigned

Labels

1.7.0bugSomething isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@phu0ngng@ptrendx@deepakn94@ksivaman