[windows] Fix FindHIP.cmake search for hipconfig. #583
Conversation
|
cc @jammm |
|
Tested more and PyTorch specifically wants the .exe versions of these scripts. It is not equipped to run the .bat versions.
|
| if(NOT UNIX) | ||
| get_filename_component(HIPCONFIG_EXECUTABLE_EXT ${HIP_HIPCONFIG_EXECUTABLE} EXT) | ||
| - if(NOT HIPCONFIG_EXECUTABLE_EXT STREQUAL ".bat") | ||
| + if(NOT (HIPCONFIG_EXECUTABLE_EXT STREQUAL ".bat" OR HIPCONFIG_EXECUTABLE_EXT STREQUAL ".exe")) | ||
| set(HIP_HIPCONFIG_EXECUTABLE "${HIP_HIPCONFIG_EXECUTABLE}.bat") | ||
| set(HIP_HIPCC_EXECUTABLE "${HIP_HIPCC_EXECUTABLE}.bat") | ||
| endif() |
There was a problem hiding this comment.
Maybe we could remove this whole section of code and only use .exe? I guess we'd need to see if projects other than PyTorch are expecting the .bat file. Another fix would be to let it continue to use the .bat, but then teach PyTorch to run that instead of a regular executable.
get_filename_component(HIPCONFIG_EXECUTABLE_EXT ${HIP_HIPCONFIG_EXECUTABLE} EXT)
if(NOT HIPCONFIG_EXECUTABLE_EXT STREQUAL ".bat")
set(HIP_HIPCONFIG_EXECUTABLE "${HIP_HIPCONFIG_EXECUTABLE}.bat")
set(HIP_HIPCC_EXECUTABLE "${HIP_HIPCC_EXECUTABLE}.bat")
endif()There was a problem hiding this comment.
I don't understand the code at ROCm/hip@73b520a and ROCm/hip@952028a. Maybe those commits predate having .exe files and only having .bat or something? Can't tell from the empty commit messages. I'll propose we just remove this file extension code and take what find_program returns to us, which should be .exe.
There was a problem hiding this comment.
I made that change and sent ROCm/hip#3789 upstream.
ScottTodd
changed the title
| find_path( | ||
| HIP_ROOT_DIR | ||
| - NAMES bin/hipconfig | ||
| + NAMES bin/hipconfig${CMAKE_EXECUTABLE_SUFFIX} |
There was a problem hiding this comment.
From the code below it seem one was looking for a .bat but following the docs CMAKE_EXECUTABLE_SUFFIX, refers to .exe on Windows. Is this the expected behavior then?
There was a problem hiding this comment.
Ah I should have read the PR description more carefully, as it is!
There was a problem hiding this comment.
I also added more context on the upstream PR I sent.
Fixes #526. Upstream PR: ROCm/hip#3789.
find_pathhere is looking for the directory containinghipconfig. On Windows,hipconfighas the.exesuffix._EXECUTABLEcode has the option of.batorexesuffixes forhipccandhipconfigas both versions exist. CMake'sfind_programonly finds the.exeversions, so that code should not append.batonto the.exe(e.g.hipconfig.exe.bat).