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

fix: preprocessor directives logic error if/else #1764

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

deepsek
Copy link

@deepsek deepsek commented Dec 19, 2024

Fix for a bug in the ck profiler code for grouped_gemm_fixed_nk

line 102, 150..
If CK_ENABLE_BF16 or CK_ENABLE_INT8 is not defined and CK_ENABLE_F16 is defined, the code would still compile because it's still syntactically correct due to a preceding if condition for the if/else block.
But the logic breaks.

Couple of ways to solve this that I'm thinking of:

  1. break down the if/else block and keep them as separate if/else statements with preprocessor macro since the args are on user inputs.
  2. Just remove the preprocessor directives all together. I'm leaning toward this because it's user facing code, lower lvl fns will/should handle the wrong input if someone requests bf16 when lib doesn't support it.
  3. just move the preprocessors into the if else conditions, that way the codeblock won't do anything. maybe add a #error for each within to handle it better

I've implemented the second solution. But we can choose another approach if preferred.

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.

1 participant