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

ggml : reduce code duplication with helper macros for local variables #309

Merged
merged 4 commits into from
Jul 2, 2023

Conversation

goerch
Copy link
Contributor

@goerch goerch commented Jun 26, 2023

Start of this edit. Would like to get some guidance on how to proceed...

Remarks:

  • Checked with clang on Windows 11 for unused variables in release configuration
  • There are probably some differences in debug and release configuration due to the usage of assert
  • Can't check Accelerate
  • Not sure if I'm able to test OpenCL
  • Definitely can't test CUDA

@slaren
Copy link
Collaborator

slaren commented Jun 26, 2023

Not sure if @ggerganov agrees, but I think that the best way to do this may be a simple macro that has all the variables for the 3 tensors src0/src1/dst, with casts to void (like the UNUSED macro) to silence unused warnings. I trust in the compiler's ability to remove actually unused variables. No need to complicate it.

@goerch
Copy link
Contributor Author

goerch commented Jun 26, 2023

Not sure if @ggerganov agrees, but I think that the best way to do this may be a simple macro that has all the variables for the 3 tensors src0/src1/dst, with casts to void (like the UNUSED macro) to silence unused warnings. I trust in the compiler's ability to remove actually unused variables. No need to complicate it.

Thanks, then I'd tend to add the UNUSED macro to GGML_DEFINE_LOCALS.

@ggerganov
Copy link
Owner

Yes, what @slaren said - let's always unpack all 4 array elements of ne and nb to simplify.
Also, add an extra macro to unpack src0, src1 and dst all at once since this case is very commonly used

@goerch goerch changed the title [WIP] Fix #292 Fix #292 Jun 27, 2023
@goerch
Copy link
Contributor Author

goerch commented Jun 27, 2023

The changes in ggml.c look reasonable to me. Do you want me to touch other files of the code base, too?

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

Thanks - great change!

We can also apply similar simplification in the GPU backend code in other PRs

@ggerganov ggerganov changed the title Fix #292 ggml : reduce code duplication with helper macros for local variables Jul 2, 2023
@ggerganov ggerganov merged commit 66ea403 into ggerganov:master Jul 2, 2023
@goerch goerch deleted the fix-#292 branch September 20, 2023 07:23
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