-
Notifications
You must be signed in to change notification settings - Fork 753
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
[SYCL][Joint Matrix Tests] Add fill/store/apply tests for 16x16x16, 32x64x16 #12629
[SYCL][Joint Matrix Tests] Add fill/store/apply tests for 16x16x16, 32x64x16 #12629
Conversation
515eb0d
to
fadd8af
Compare
fadd8af
to
61c5b46
Compare
61c5b46
to
3069fc9
Compare
21297f1
to
ba6703b
Compare
ba6703b
to
d6d6278
Compare
d6d6278
to
64fd280
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
@@ -122,7 +122,8 @@ int main() { | |||
float>(); | |||
} | |||
} | |||
if (combinations[i].atype == matrix_type::sint8) { | |||
if (combinations[i].atype == matrix_type::sint8 && |
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.
I noticed 2 combinations satisfy this condition, so the same tests are running 2 times on PVC, hence added one more condition, so only 1 combination satisfies the condition.
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.
LGTM
void matrix_multiply_ref(Ta *A, Tb *B, Tc *C, int M, int N, int K, | ||
bool transpose_c = false, bool colmajor_a = false, | ||
bool colmajor_b = false) { | ||
bool colmajor_b = false, F &&lambda = {}) { |
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.
This function got very complicated instead of just adding a new one like I suggested: matrix_multiply_apply_ref as these are two different things.
To avoid code duplication, matrix_multiply_apply_ref would call matrix_multiply_ref
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.
If matrix_multiply_apply_ref would call matrix_mulitply_ref, then multiplication and apply would not be fused, right? Then what is the point of adding matrix_multiply_apply_ref? Why not just use matrix_multiply_ref and then matrix_apply_ref, as I initially did?
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.
I thought you did not want to fuse. In this case the point is to use modularity.
A new function with fusion would be perfect. We could outline the duplicated code in an another function.
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.
Well, I did not want to create a new fused version, as I thought that would be too much just for one test case. But then I figured I can make a fused version out of the existing multiply function with just a few simple changes. So I decided to produce a fused version without creating more entities. Yes, function naming is not ideal now, but overall, I don't consider it to be very complicated and it allows both multiplication and apply in a fused way, while if test requires only multiplication, call can be kept simple, as apply-related parameters are optional.
@intel/llvm-gatekeepers , please, merge. all green. |
No description provided.