Add Feature: BLAS GEMM #217
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new BLAS GEMM implementation and a Global Average Pool node while also refactoring testing code for improved clarity and maintainability. Key changes include:
- Adding a new BLAS GEMM branch (using OpenBLAS) and its corresponding code paths.
- Introducing a GlobalAveragePool node and integrating it into the parser.
- Refactoring test files for image preprocessing and inference (renaming tests and removing kernel flipping code).
Reviewed Changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_image_to_guess.cpp | Refactored image preprocessing/inference tests using new helpers. |
| tests/test_ImageNet.cpp | Updated tests with a new default parameter for model JSON path and clearer test names. |
| src/nodes/global_avg_pool.cpp | Added implementation of the Global Average Pool node. |
| src/nodes/conv.cpp | Removed the kernel flipping functionality as intended. |
| src/include/nodes/global_avg_pool.hpp | Added the header for the Global Average Pool node. |
| src/include/nodes/conv.hpp | Removed obsolete flip_kernel declaration. |
| src/datastructures/tensor_operations_module.tpp | Introduced a new branch for BLAS-based GEMM. |
| src/datastructures/tensor_operation_functions.tpp | Added OpenBLAS GEMM implementation and required includes. |
| src/backend/mml_parser.cpp | Integrated GlobalAveragePool node support in the parser. |
Files not reviewed (3)
- CMakeLists.txt: Language not supported
- Makefile: Language not supported
- src/include/modularml: Language not supported
Comments suppressed due to low confidence (1)
src/nodes/global_avg_pool.cpp:94
- [nitpick] Consider clarifying or removing the ambiguous 'FIX' comment since a clearly named array_mml (in_index) is already used for indexing, to improve code readability.
// *** FIX: use a named array_mml to index ***
This was
linked to
issues
Apr 25, 2025
willayy
reviewed
Apr 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR closes multiple issues. See closes.
Adds: BLAS implementation that can be used similar to the AVX versions
Adds: New model to test imagenet against
Removes: flipping the kernel
Remoces: duplicate code
Issue
Closes #216 #215 #214 #213