-
Notifications
You must be signed in to change notification settings - Fork 1.1k
update kt-kernel #1530
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
update kt-kernel #1530
Conversation
Summary of ChangesHello @ovowei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the development workflow by introducing automated code quality checks via Git hooks and streamlining dependency management. Crucially, it integrates high-performance CPU kernels from the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces kt-kernel, including build infrastructure, dependencies, git hooks, and a significant amount of performance-oriented C++ code adapted from llamafile for CPU-based kernel operations. The changes are extensive and set up a new component in the repository.
My review focuses on the new scripts and the general structure of the added C++ code. I've identified a few issues:
- A logic error in the
commit-msggit hook that would prevent developers from using the documented commit message format for scopes. - A robustness issue in the
pre-commithook when handling Python files with spaces in their names. - A recurring typo in copyright notices across many of the new C++ files.
- Formatting problems in function declarations within
sgemm.hthat affect readability.
Overall, the additions are substantial. Addressing these points will improve the developer experience and code quality.
| [type](scope)?: subject | ||
|
|
||
| Examples: | ||
| [feat]: add new feature | ||
| [fix(parser)]: handle edge case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message format description and examples are inconsistent with the validation regex.
- The format description on line 41 is missing the optional breaking change indicator
!. - The example on line 45 for a commit with a scope,
[fix(parser)]: ..., will be rejected by the regex, which expects the scope to be outside the square brackets, like[fix](parser): ....
To ensure the hook works as intended and the guidance is clear, both the description and the example should be aligned with the regex.
| [type](scope)?: subject | |
| Examples: | |
| [feat]: add new feature | |
| [fix(parser)]: handle edge case | |
| [type](scope)?[!]?: subject | |
| Examples: | |
| [feat]: add new feature | |
| [fix](parser): handle edge case |
| PY_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.py$' || true) | ||
| if [ -n "$PY_FILES" ]; then | ||
| echo "[pre-commit] running black on staged python files..." >&2 | ||
| $BLACK_BIN $PY_FILES | ||
| else | ||
| # Optionally format all python files; comment out if not desired | ||
| # $BLACK_BIN "$REPO_ROOT" | ||
| : | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current method of passing python files to black is not robust. It will fail for filenames containing spaces due to word splitting, and it could potentially exceed the shell's argument length limit if many files are staged. Using mapfile (a bash 4+ feature) to read files into an array is a more robust approach that correctly handles spaces in filenames.
| PY_FILES=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\.py$' || true) | |
| if [ -n "$PY_FILES" ]; then | |
| echo "[pre-commit] running black on staged python files..." >&2 | |
| $BLACK_BIN $PY_FILES | |
| else | |
| # Optionally format all python files; comment out if not desired | |
| # $BLACK_BIN "$REPO_ROOT" | |
| : | |
| fi | |
| mapfile -t PY_FILES < <(git diff --cached --name-only --diff-filter=ACM | grep -E '\.py$' || true) | |
| if [ ${#PY_FILES[@]} -gt 0 ]; then | |
| echo "[pre-commit] running black on staged python files..." >&2 | |
| "$BLACK_BIN" "${PY_FILES[@]}" | |
| fi |
| @@ -0,0 +1,25 @@ | |||
| // Adapted from | |||
| // https://github.com/Mozilla-Ocho/llamafile/blob/0.8.8/llamafile/bench.h | |||
| // Copyrigth 2024 Mozilla Foundation. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool iqk_mul_mat(long, long, long,int, const void*, long, int, const void*, long,float*, long, int, int); | ||
| bool iqk_mul_mat_zen4(long, long, long,int, const void*, long, int, const void*, long,float*, long, int, int); | ||
| bool iqk_mul_mat_arm82(long, long, long,int, const void*, long, int, const void*, long,float*, long, int, int); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function declarations in this block have formatting issues, with missing spaces between types and some parameter names (e.g., long,int,). This harms readability and could be problematic for some tools or compilers. It's recommended to format them consistently.
| bool iqk_mul_mat(long, long, long,int, const void*, long, int, const void*, long,float*, long, int, int); | |
| bool iqk_mul_mat_zen4(long, long, long,int, const void*, long, int, const void*, long,float*, long, int, int); | |
| bool iqk_mul_mat_arm82(long, long, long,int, const void*, long, int, const void*, long,float*, long, int, int); | |
| bool iqk_mul_mat(long, long, long, int, const void*, long, int, const void*, long, float*, long, int, int); | |
| bool iqk_mul_mat_zen4(long, long, long, int, const void*, long, int, const void*, long, float*, long, int, int); | |
| bool iqk_mul_mat_arm82(long, long, long, int, const void*, long, int, const void*, long, float*, long, int, int); |
No description provided.