Skip to content

Conversation

@futz12
Copy link
Contributor

@futz12 futz12 commented Dec 29, 2025

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2025

Codecov Report

❌ Patch coverage is 96.49805% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.91%. Comparing base (06c4da1) to head (3b75b93).

Files with missing lines Patch % Lines
src/layer/vulkan/matmul_vulkan.cpp 96.49% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #6470    +/-   ##
========================================
  Coverage   95.90%   95.91%            
========================================
  Files         845      846     +1     
  Lines      265399   265600   +201     
========================================
+ Hits       254531   254741   +210     
+ Misses      10868    10859     -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds Vulkan shader support for matrix multiplication operations to the ncnn framework. The implementation provides GPU acceleration for matmul operations using Vulkan compute shaders.

  • Implements a new Vulkan compute shader for matrix multiplication with support for transposed matrices and batched operations
  • Adds MatMul_vulkan class following the established pattern for Vulkan layer implementations
  • Handles multiple input dimensions (1D through 4D tensors) with appropriate broadcasting

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File Description
src/layer/vulkan/shader/matmul.comp New Vulkan compute shader implementing matrix multiplication with local memory optimization and support for various tensor dimensions
src/layer/vulkan/matmul_vulkan.h Header file declaring the MatMul_vulkan class with standard Vulkan layer interface
src/layer/vulkan/matmul_vulkan.cpp Implementation of MatMul_vulkan class handling dimension calculations, tensor broadcasting, and pipeline dispatch

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 132 to 135
if (in_m0)
a0 = lfp(buffer_ld1(a_blob_data, a_base + m0 * p.a_w + ak));
if (in_m1)
a1 = lfp(buffer_ld1(a_blob_data, a_base + (m0 + 1) * p.a_w + ak));
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The statements inside these if blocks are missing proper indentation. They should be indented to match the code style used elsewhere in the file.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

solve

Comment on lines 148 to 151
if (transB == 0)
b_idx0 = b_base + bk * p.b_w + n0;
else
b_idx0 = b_base + n0 * p.b_w + bk;
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The statements inside these if blocks are missing proper indentation. They should be indented to match the code style used elsewhere in the file.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

solve

@@ -0,0 +1,28 @@
// Copyright 2025 pchar.cn
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The copyright format is inconsistent with other files by the same author. Other files in this directory use "Copyright 2025 Futz12 <pchar.cn>" (e.g., layernorm_vulkan.h, rmsnorm_vulkan.h). Consider using the same format for consistency.

Suggested change
// Copyright 2025 pchar.cn
// Copyright 2025 Futz12 <pchar.cn>

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

solve

@@ -0,0 +1,411 @@
// Copyright 2025 pchar.cn
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The copyright format is inconsistent with other files by the same author. Other files in this directory use "Copyright 2025 Futz12 <pchar.cn>" (e.g., layernorm_vulkan.cpp, rmsnorm_vulkan.cpp). Consider using the same format for consistency.

Suggested change
// Copyright 2025 pchar.cn
// Copyright 2025 Futz12 <pchar.cn>

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,384 @@
// Copyright 2025 pchar.cn
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The copyright format is inconsistent with other shader files by the same author. Other shader files in this directory use "Copyright 2025 Futz12 <pchar.cn>" (e.g., layernorm_.comp, rmsnorm_.comp). Consider using the same format for consistency.

Suggested change
// Copyright 2025 pchar.cn
// Copyright 2025 Futz12 <pchar.cn>

Copilot uses AI. Check for mistakes.
Comment on lines 94 to 118
bool in_m0; // = (m0 < p.out_h);
bool in_m1; // = (m0 + 1 < p.out_h);
bool in_n0; // = (n0 < p.out_w);
bool in_n1; // = (n0 + 1 < p.out_w);

if (m0 < p.out_h)
in_m0 = true;
else
in_m0 = false;

if (m0 + 1 < p.out_h)
in_m1 = true;
else
in_m1 = false;

if (n0 < p.out_w)
in_n0 = true;
else
in_n0 = false;

if (n0 + 1 < p.out_w)
in_n1 = true;
else
in_n1 = false;

Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The boolean assignments can be simplified. Instead of using if-else statements to assign true/false, directly use the comparison result.

Suggested change
bool in_m0; // = (m0 < p.out_h);
bool in_m1; // = (m0 + 1 < p.out_h);
bool in_n0; // = (n0 < p.out_w);
bool in_n1; // = (n0 + 1 < p.out_w);
if (m0 < p.out_h)
in_m0 = true;
else
in_m0 = false;
if (m0 + 1 < p.out_h)
in_m1 = true;
else
in_m1 = false;
if (n0 < p.out_w)
in_n0 = true;
else
in_n0 = false;
if (n0 + 1 < p.out_w)
in_n1 = true;
else
in_n1 = false;
bool in_m0 = (m0 < p.out_h);
bool in_m1 = (m0 + 1 < p.out_h);
bool in_n0 = (n0 < p.out_w);
bool in_n1 = (n0 + 1 < p.out_w);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

solve

Comment on lines 158 to 161
if (transB == 0)
b_idx1 = b_base + bk * p.b_w + (n0 + 1);
else
b_idx1 = b_base + (n0 + 1) * p.b_w + bk;
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The statements inside these if blocks are missing proper indentation. They should be indented to match the code style used elsewhere in the file.

Copilot uses AI. Check for mistakes.
@tencent-adm
Copy link
Member

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ futz12
❌ nihui
You have signed the CLA already but the status is still pending? Let us recheck it.

@nihui nihui closed this Jan 3, 2026
@nihui nihui reopened this Jan 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants