Repository navigation
[MIOpen] Convert the OpenCL conv solver conv_ocl_dir2D11x11 to hip solver - #6164
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
randyspauldingamd
left a comment
There was a problem hiding this comment.
I would like a bit more rigor given how frequently broken kernels have gotten through. Could you please do some elementwise comparisons against the original code and share the results?
I understand the concern. While there is no direct element-wise comparison between the OCL and HIP outputs, there is an indirect one — both solvers are independently verified against the same CPU reference in the unit test suite, which is mathematically equivalent. Here is why the unit tests are sufficient:
Since both unit tests pass on all three datatypes with the same test cases, this provides a mathematically rigorous bound on the deviation between the two implementations — and also illustrates exactly why having these unit tests matters. |
AnaghaRaoAMD
left a comment
There was a problem hiding this comment.
LGTM, some CI failures need attention
Thanks so much for the comments. I have checked about the CI failure. It seems that the failure is unrelated to this PR. It refers to ConvHipImplicitGemmGroupWrwXdlops on gfx942, which is neither touched by this PR nor present in any of the modified fdb files. The invalid perf config in gfx942130.db.txt should be a pre-existing issue in the performance database. |
While you're correct in regard to elementwise comparisons, I remind everyone that these tests do not do that. They use RRMSE, which can hide many significant elementwise inaccuracies. In fact, this is the purpose of using it. It is dangerous to use as a standalone metric though, and what I'm asking for is a sanity check to gauge how bad this is in practice. You may have overgeneralized in your example. I worked through what I think you had in mind. First, go back to the base definition of triangle inequality by defining: You're trying to relate RRMSE(|x - y|) to RRMSE(|x|) and RRMSE(|y|) since we have information about the latter two. Lets try it! Note, M[v] refers to the 'max' term. For readability I'll move the M's to the right and just look at RMSE, and also skip writing unneeded absolute value symbols: Now apply RMSE to the triangle inequality: After squaring both sides and multiplying by N, all the sum-of-squares terms cancel and we are left with: Which is true by inspection and, assuming my method and algebra are correct, shows that the triangle inequality does not help when comparing RRMSE. We should really be looking at the histograms for all three sets, |
|
Hi @randyspauldingamd, you're right about the proof. The triangle inequality does not transfer to RRMSE the way I described — thank you for working through the math carefully. |
|
Hi @randyspauldingamd , please check the attacked results of element comparisons. Comparison Analysis: OCL vs HIP Solver vs CPU Reference Total output elements: 81 (I dumped the result to a text file and each line contains one element, datatype fp32) --- HIP solver vs CPU reference --- The differences are all at the last displayed significant digit (e.g. 5.68386e+18 vs 5.68387e+18). The one extra diff for OCL is at line 39 (3.95323e+19 vs 3.95324e+19) — the HIP solver happens to match the CPU reference exactly on that value while OCL doesn't. Overall both solvers agree with CPU to within floating-point rounding tolerance, and all tests pass. The small divergences are normal for GPU convolutions due to non-associative FP accumulation order. --- OCL solver vs HIP solver --- Summary OCL vs CPU RMS: 2.757795e+13 |
The accuracy and performance comparison tests of the ocl and hip solver show that they are equivalent to each other. About the concern of the current test infrastructure, that worth to address separately.
The previous merge resolution was incorrect. Develop's FindDB changes (PR #6164) removed ConvOclDirectFwd11x11 entries, while our branch removed ConvOclDirectFwd1x1 entries. These are independent changes that both need to be present. Rebuild the FindDB files by starting from develop's version and stripping ConvOclDirectFwd1x1 entries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lver (#6164) ## Motivation Convert the OpenCL kernel and solver for the 11×11 direct forward convolution (ConvOclDirectFwd11x11) to hip solver as part of the ongoing effort to replace OpenCL solvers/kernels with hip solvers/kernels in MIOpen. ## Technical Details - Removed the OpenCL solver conv_ocl_dir2D11x11.cpp and its kernel MIOpenConvFwd_LxL_11.cl (1466 lines). - Added a replacement HIP solver conv_hip_dir2D11x11.cpp and HIP kernel MIOpenConvFwd_LxL_11.cpp, which is a direct port of the OpenCL kernel supporting the same 11×11, stride-4 tiled forward convolution algorithm. - Replaced ConvOclDirectFwd11x11 with ConvHipDirectFwd11x11 in the solver registry (solver.cpp) and solver list (mlo_dir_conv.cpp). - Updated all find-db files (HIP and OpenCL) that referenced the old solver. - Extended GPU support to gfx90x, gfx94x, gfx95x, gfx103x, gfx110x, gfx115x, and gfx120x. - Updated docs to reflect the renamed environment variable (MIOPEN_DEBUG_CONV_DIRECT_HIP_FWD11X11). ## Test Plan - Run GPU_UnitTestConvSolverHipDirectFwd11x11 for FP16, BFP16, and FP32 on supported hardware. - Run CPU_UnitTestConvSolverHipDirectFwd11x11DevApplicability_NONE for device applicability checks. ## Test Result - All unit tests pass on gfx94x (FP32 tolerance set to 3.0× ULP to account for accumulation differences across architectures). - CI test pending ## Submission Checklist - Removed OpenCL kernel and solver - Added replacement HIP kernel and solver - Updated solver registry with ID slot preserved - Updated find-db files - Added unit tests for the new HIP solver - Updated documentation --------- Co-authored-by: BradPepersAMD <Brad.Pepers@amd.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…hip rename PR #6164 on develop renamed solver/conv/conv_ocl_dir2D11x11.cpp to conv_hip_dir2D11x11.cpp (OpenCL solver converted to HIP). The uplift branch picked up the renamed source file but not the corresponding CMakeLists.txt update, causing: CMake Error at src/CMakeLists.txt:870 (add_library): Cannot find source file: solver/conv/conv_ocl_dir2D11x11.cpp This patch updates CMakeLists.txt to reference the new filename, matching the state on develop.
Motivation
Convert the OpenCL kernel and solver for the 11×11 direct forward convolution (ConvOclDirectFwd11x11) to hip solver as part of the ongoing effort to replace OpenCL solvers/kernels with hip solvers/kernels in MIOpen.
Technical Details
algorithm.
Test Plan
Test Result
Submission Checklist