Skip to content

Avoid unused constant warnings #2029

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

set-soft
Copy link
Contributor

  • They are used by some targets, not all
  • So I moved them to each target, expanding the value

- They are used by some targets, not all
- So I moved them to each target, expanding the value
@ggerganov
Copy link
Member

I think it is better the other way around - use nb everywhere and avoid QK_X
Makes things more consistent

@set-soft
Copy link
Contributor Author

And how do you plan to stop the warning about unused constant for the targets where nb isn't used?
I can try moving more code inside the ifs ... The patch is oriented to generate 0 warnings on my workflow.

BTW this reminds me something I noticed: the code is full of compile time decisions, but I think a lot of them should be done at run time.
I agree that ARM specific code should be separated from x86_64 code (IMHO in separated source files), but trying to separate SSSE3 and the various AVX flavors doesn't look like a good idea to me:

  1. Because creating a binary to distribute becomes a nightmare. As an example: I compiled the code on GitHub CI/CD and got code that didn't run on my system, why? because GitHub uses Xeon processors. Of course I realized how to avoid it. But is an example.
  2. Because it doesn't always work as expected. In the case of my Ryzen 5 2600 the conditionals choose AVX over SSSE3, but if I compile the code with only SSSE3 enabled I get better performance. So selecting it at runtime could be better.

This topic was already discussed? sorry for mixing stuff is just that I remembered it because of the conditionals triggering warnings for some cases and not for others. Which reminds me another thing that could help. Sometimes the pre-processor conditionals aren't a good idea. Using regular C code if (DEFINITION) { ... code 1 ... } else { ... code 2 ... } is better, as long as code 1 and 2 can be interpreted by the compiler it will just remove the unused code, like the pre-processor, but with a major advantage: you test code 1 and 2 syntax when DEFINITION is true and when definition is false. Also: things like constants that aren't used in code 1, but used in code 2 won't generate a warning because they are used indeed (even when optimized away). I don't claim this can be always applied, but I'm quite sure it can be applied in more than one place.

@ggerganov
Copy link
Member

The runtime instruction set detection was discussed some time ago here: ggml-org/ggml#86 (comment)

And how do you plan to stop the warning about unused constant for the targets where nb isn't used?

We can add UNUSED(nb) in the branches where it is not used

@mofosyne mofosyne added refactoring Refactoring Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix labels May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants