Skip to content

Renames NVIDIA GPU-architecture flags to avoid name clashes with WinAPI #10736

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 10, 2024
Merged

Renames NVIDIA GPU-architecture flags to avoid name clashes with WinAPI #10736

merged 4 commits into from
Dec 10, 2024

Conversation

aendk
Copy link
Contributor

@aendk aendk commented Dec 9, 2024

For example, CC_PASCAL is a conflict, as it is both a GPU architecture and a WinAPI pascal compiler flag.

@aendk
Renames NVIDIA GPU-architecture flags to avoid name clashes with WinA…
b96d382 
…PI. (e.g. CC_PASCAL, GPU architecture or WinAPI pascal compiler flag?)
@github-actions github-actions bot added Nvidia GPU Issues specific to Nvidia GPUs ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Dec 9, 2024
@aendk aendk mentioned this pull request Dec 9, 2024
Closed
JohannesGaessler
JohannesGaessler reviewed Dec 9, 2024
View reviewed changes
ggml/src/ggml-cuda/common.cuh Outdated
#define CC_TURING 750
#define CC_AMPERE 800
#define GGML_CUDA_CC_PASCAL 600
#define GGML_CUDA_MIN_CC_DP4A 610 // minimum compute capability for __dp4a, an intrinsic for byte-wise dot products
Copy link
Collaborator

@JohannesGaessler JohannesGaessler Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since you're already renaming the macros, can you change this to GGML_CUDA_CC_DP4A? CUDA code is forwards-compatible so it should be implicitly clear that this is the minimum compute capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

JohannesGaessler
JohannesGaessler requested changes Dec 9, 2024
View reviewed changes
ggml/src/ggml-sycl/vecdotq.hpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a CUDA file but a SYCL file. It should not be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, fixed it.

JohannesGaessler
JohannesGaessler reviewed Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@JohannesGaessler JohannesGaessler left a comment

Choose a reason for hiding this comment

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

Please also change the compute capability macros for the non-NVIDIA architectures in order to be consistent.

@JohannesGaessler
Copy link
Collaborator

Just so there is no confusion: I don't mean for you to touch any of e.g. the SYCL code, I'm only asking you to rename the macros like CC_GCN4 that are below the NVIDIA-related macros.

@aendk
Copy link
Contributor Author

aendk commented Dec 9, 2024

Do you have a suggestion for the prefix? Is the GGML_ prefix sufficient?

@JohannesGaessler
Copy link
Collaborator

My opinion is that GGML_CUDA would be preferable for consistency since these macros do not make sense outside of the context of the CUDA backend.

@aendk
Copy link
Contributor Author

aendk commented Dec 9, 2024

@JohannesGaessler feel free to look over it again, I implemented your suggestions.

@JohannesGaessler JohannesGaessler removed the SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language label Dec 9, 2024
@JohannesGaessler JohannesGaessler merged commit 750cb3e into ggml-org:master Dec 10, 2024
47 checks passed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Dec 20, 2024
@aendk @arthw
CUDA: rename macros to avoid conflicts with WinAPI (ggml-org#10736)
3712295 
* Renames NVIDIA GPU-architecture flags to avoid name clashes with WinAPI. (e.g. CC_PASCAL, GPU architecture or WinAPI pascal compiler flag?)

* Reverts erroneous rename in SYCL-code.

* Renames GGML_CUDA_MIN_CC_DP4A to GGML_CUDA_CC_DP4A.

* Renames the rest of the compute capability macros for consistency.
tinglou pushed a commit to tinglou/llama.cpp that referenced this pull request Feb 13, 2025
@aendk @tinglou
CUDA: rename macros to avoid conflicts with WinAPI (ggml-org#10736)
73cbc2f 
* Renames NVIDIA GPU-architecture flags to avoid name clashes with WinAPI. (e.g. CC_PASCAL, GPU architecture or WinAPI pascal compiler flag?)

* Reverts erroneous rename in SYCL-code.

* Renames GGML_CUDA_MIN_CC_DP4A to GGML_CUDA_CC_DP4A.

* Renames the rest of the compute capability macros for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohannesGaessler JohannesGaessler JohannesGaessler approved these changes

Assignees
No one assigned
Labels
ggml changes relating to the ggml tensor library for machine learning Nvidia GPU Issues specific to Nvidia GPUs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aendk @JohannesGaessler