Adding accuracy check in benchmarking#4
Conversation
|
@RAJTripathi3030 I love the energy, haha. An example would be "[bench] Implement numerical correctness checker for user provided gemm fptrs". Thanks for the patch, this is a great start! |
| DEVBLAS_INT_T check_N = std::min(N, (DEVBLAS_INT_T)256); | ||
| DEVBLAS_INT_T check_K = std::min(K, (DEVBLAS_INT_T)256); | ||
|
|
||
| devblas_gemm_config_t small_config = *config; |
There was a problem hiding this comment.
This just derefed the original C side config pointer and mutated it, so now we have lost the actual problem size passed by the user and substituted it with the reference size.
Ideally what would be done is to assign a new variable to cppConfig. The '=' operator will invoke the copy constructor and you won't be modifying the original config.
We should ideally not operate on C side structure in the C++ code and convert early to cpp structures.
|
|
||
| std::vector<T> C_ref(check_M * ldc, T{0}); | ||
| std::vector<T> C_test(check_M * ldc, T{0}); | ||
|
|
There was a problem hiding this comment.
We don't need two vectors here. We can overwrite the original C vector allocated for the full problem for the user function and allocate a small vector to hold golden output.
C will be overwritten once it actually enters the benchmark anyway, so it is safe to use it here.
| std::vector<T> C_ref(check_M * ldc, T{0}); | ||
| std::vector<T> C_test(check_M * ldc, T{0}); | ||
|
|
||
| auto ref_cfg = types::gemm_config_to_cpp(&small_config); |
There was a problem hiding this comment.
This is redundant. ref_cfg should have been constructed by assignment from cppConfig. It might make more sense to have a setter function for logical dims (or some other mutation function you see fit) to be able to modify them, or to have the copy constructor implemented to handle that.
| add_library(devblas SHARED | ||
| kernels/naive_gemm.cc | ||
| kernels/tiled_gemm.cc | ||
| kernels/my_sgemm.c |
There was a problem hiding this comment.
A new implementation should not be added into the code unless it contains a kernel distinct and necessary enough to justify ite addition.
There was a problem hiding this comment.
This looks like a user side kernel anyway. It should stay user side and not be added to the library.
|
|
||
| if (!passed) { | ||
| std::cerr << "[BENCH ABORTED] Fix your kernel first.\n"; | ||
| return; |
There was a problem hiding this comment.
We'd need slightly more informative and slightly less angry logging :)
| // Verifying User's config for calculative correctness | ||
| DEVBLAS_INT_T check_M = std::min(M, (DEVBLAS_INT_T)256); | ||
| DEVBLAS_INT_T check_N = std::min(N, (DEVBLAS_INT_T)256); | ||
| DEVBLAS_INT_T check_K = std::min(K, (DEVBLAS_INT_T)256); |
There was a problem hiding this comment.
256 should not be hard-coded. Either put it as a compile time constant tied to a variable name or make it controllable through the config.
2 participants
Hey there vin-dev, I have added the accuracy/reference check feature in bench.cc file.
PS: Accuracy ki spelling galat ho gayi branch me abhi dekha maine so pls ignore that :)