[UR][HIP] Enable kernel finalization using comgr #940
Conversation
For kernel fusion support for hip, we need to finalize the kernels using comgr. The patch finalizes tagged binaries during buildProgram before handing it over to the hip runtime. Signed-off-by: Victor Lomuller <victor@codeplay.com>
Naghasan
force-pushed
the
victor/kernel-fusion-amd
branch
from
2011911
to
2fd9dea
Compare
Naghasan
changed the title
|
@JackAKirk could have a wee look please ? (I think you are one of the code owner) This is the UR part of the patch you approved in the main intel/llvm side |
sure |
|
Thanks |
| @@ -18,7 +18,7 @@ set(UR_HIP_INCLUDE_DIR "${UR_HIP_ROCM_DIR}/include") | |||
| set(UR_HIP_HSA_INCLUDE_DIR "${UR_HIP_ROCM_DIR}/hsa/include") | |||
|
|
|||
| # Set HIP lib dir | |||
| set(UR_HIP_LIB_DIR "${UR_HIP_ROCM_DIR}/hip/lib") | |||
| set(UR_HIP_LIB_DIR "${UR_HIP_ROCM_DIR}/lib") | |||
There was a problem hiding this comment.
CC @kbenzie for awareness. This change implies UR_HIP_ROCM_DIR semantic has changed. I guess this might affect the CMAKE gymnastics with regard to pointing unified-runtime/intel-llvm to the correct rocm.
There was a problem hiding this comment.
Thanks, this looks more logical to me but I'm not well versed in finding rocm CMake game.
There was a problem hiding this comment.
libs found in hip/lib are also found in lib via a symlink. But logically, the path is no longer HIP but ROCM (which include ROCM). Hence the simple change here.
| @@ -259,6 +412,16 @@ UR_APIEXPORT ur_result_t UR_APICALL urProgramCreateWithBinary( | |||
|
|
|||
| // TODO: Set metadata here and use reqd_work_group_size information. | |||
| // See urProgramCreateWithBinary in CUDA adapter. | |||
| if (pProperties) { | |||
There was a problem hiding this comment.
@jchlanda I was just wondering whether this affects your work on reqd_work_group_size metadata?
There was a problem hiding this comment.
Yeap, I've added essentially the same snippet to set the metadata.
I guess we should be OK though, whichever patch gets in first will cause a merge conflict for the other, but that should be easy to resolve.
There was a problem hiding this comment.
I suppose, I reused the structure from the CUDA adapter. I suppose you did the same, so that's just a matter of filling in the if condition
There was a problem hiding this comment.
Agreed, git will likely resolve it for us. Not to worry.
| ur_result_t ur_program_handle_t_::finalizeRelocatable() { | ||
| #ifndef SYCL_ENABLE_KERNEL_FUSION | ||
| assert(false && "Relocation only available with fusion"); | ||
| return UR_RESULT_ERROR_UNKNOWN; |
There was a problem hiding this comment.
This line will never be reached right? because of the above assert being set to false.
Also it would be better to use a backend specific error that then gets automatically passed to a sycl extension, using setErrorMessage, like here: https://github.com/intel/llvm/pull/10953/files
There was a problem hiding this comment.
This line will never be reached right?
In a normal setup, correct. Hence the false here.
In order to be reached, you would need to either enable fusion in intel/llvm and actively disable it in the adapter or use the finalize property in some other patch. Either way, you need to actively change something in the build (cmake changes) or add a patch (which if tested should fail CI) to trigger this.
I can add a setErrorMessage if you care about a non sycl focus use of the adapter, otherwise I don't think it is that useful.
There was a problem hiding this comment.
Yeah it's fine to leave it as it is.
|
Unless there is more comments, can we have this merged please ? thanks |
Updating UR in intel/llvm is currently blocked by intel/llvm#11312 - I'm hoping to get that resolved today but want to do that before merging any more adapter changed to UR, things are already kinda complicated. I'll add it to the list to merge once things are unblocked. |
For kernel fusion support for hip, we need to finalize the kernels using comgr. The patch finalizes tagged binaries during buildProgram before handing it over to the hip runtime.
Initial / related PR intel/llvm#11003