This change switches Cycles to an opensource HIP-RT library which
implements hardware ray-tracing. This library is now used on
both Windows and Linux. While there should be no noticeable changes
on Windows, on Linux this adds support for hardware ray-tracing on
AMD GPUs.
The majority of the change is typical platform code to add new
library to the dependency builder, and a change in the way how
ahead-of-time (AoT) kernels are compiled. There are changes in
Cycles itself, but they are rather straightforward: some APIs
changed in the opensource version of the library.
There are a couple of extra files which are needed for this to
work: hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb.
There are some assumptions in the HIP-RT library about how they
are available. Currently they follow the same rule as AoT
kernels for oneAPI:
This change switches Cycles to an opensource HIP-RT library which
implements hardware ray-tracing. This library is now used on
both Windows and Linux. While there should be no noticeable changes
on Windows, on Linux this adds support for hardware ray-tracing on
AMD GPUs.
The majority of the change is typical platform code to add new
library to the dependency builder, and a change in the way how
ahead-of-time (AoT) kernels are compiled. There are changes in
Cycles itself, but they are rather straightforward: some APIs
changed in the opensource version of the library.
There are a couple of extra files which are needed for this to
work: hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb.
There are some assumptions in the HIP-RT library about how they
are available. Currently they follow the same rule as AoT
kernels for oneAPI:
- On Windows they are next to blender.exe
- On Linux they are in the lib/ folder
Performance comparison on Ubuntu 22.04.5:
```
GPU: AMD Radeon PRO W7800
Driver: amdgpu-install_6.1.60103-1_all.deb
main hip-rt
attic 0.1414s 0.0932s
barbershop_interior 0.1563s 0.1258s
bistro 0.2134s 0.1597s
bmw27 0.0119s 0.0099s
classroom 0.1006s 0.0803s
fishy_cat 0.0248s 0.0178s
junkshop 0.0916s 0.0713s
koro 0.0589s 0.0720s
monster 0.0435s 0.0385s
pabellon 0.0543s 0.0391s
sponza 0.0223s 0.0180s
spring 0.1026s 1.5145s
victor 0.1901s 0.1239s
wdas_cloud 0.1153s 0.1125s
```
Generally we try to make things work with old and new versions of a library, to make coordination of landing updates easier. But in this case it looks not so simple, and not worth doing.
So I think when the time comes to land this, we either disable WITH_CYCLES_DEVICE_HIPRT for a little bit, or coordinate with @LazyDodo to land precompiled libraries at the same time.
Generally we try to make things work with old and new versions of a library, to make coordination of landing updates easier. But in this case it looks not so simple, and not worth doing.
So I think when the time comes to land this, we either disable `WITH_CYCLES_DEVICE_HIPRT` for a little bit, or coordinate with @LazyDodo to land precompiled libraries at the same time.
I wasn't able to actually test the make deps integration because the version doesn't seem to exist. But some comments from reading through the changes.
I wasn't able to actually test the `make deps` integration because the version doesn't seem to exist. But some comments from reading through the changes.
It's build_files/cmake/config/blender_release.cmake that enables it by default for releases. Though we should still not enable it for Linux there, until after the precompiled libraries for that platform have landed.
This should still default to OFF.
It's `build_files/cmake/config/blender_release.cmake` that enables it by default for releases. Though we should still not enable it for Linux there, until after the precompiled libraries for that platform have landed.
For Linux, there will need to be additions in `build_files/build_environment/cmake/harvest.cmake` to install things.
I guess something like this, not sure if we want to just install all files or only a subset.
```
harvest(hiprt/include hiprt/include "*")
harvest(hiprt/lib hiprt/lib "*")
```
Not sure if we need to wait for it to be released, or if another version should be used.
This version doesn't seem to exist?
https://github.com/GPUOpen-LibrariesAndSDKs/HIPRTSDK/releases
Not sure if we need to wait for it to be released, or if another version should be used.
Ok, thanks. For reference, this PR is heavily WIP and will need many more changes to get it to actually build.
I asked to just add the code so we get an idea of how things should fit together, what the build options should be, etc. But not to actually test if it works, since it's not so easy to build the libs on Windows.
Ok, thanks. For reference, this PR is heavily WIP and will need many more changes to get it to actually build.
I asked to just add the code so we get an idea of how things should fit together, what the build options should be, etc. But not to actually test if it works, since it's not so easy to build the libs on Windows.
This seems to be unused currently. I guess the idea is still to dynamically load the hiprt library rather than Blender linking to it? If so I guess this variable can be removed.
To install the shared libraries along with Blender, this will need to be added in build_files/cmake/platform/platform_unix.cmake, which will copy all .so files into the bundled lib directory.
add_bundled_libraries(hiprt/lib)
For Windows this is done in source/creator/CMakeLists.txt, something like:
if(WITH_CYCLES_DEVICE_HIPRT)
windows_install_shared_manifest(
FILES ${LIBDIR}/hiprt/bin/hiprt64.dll
ALL
)
endif()
Unless there are separate release and debug libraries, in which case:
This seems to be unused currently. I guess the idea is still to dynamically load the hiprt library rather than Blender linking to it? If so I guess this variable can be removed.
To install the shared libraries along with Blender, this will need to be added in `build_files/cmake/platform/platform_unix.cmake`, which will copy all `.so` files into the bundled lib directory.
```
add_bundled_libraries(hiprt/lib)
```
For Windows this is done in `source/creator/CMakeLists.txt`, something like:
```
if(WITH_CYCLES_DEVICE_HIPRT)
windows_install_shared_manifest(
FILES ${LIBDIR}/hiprt/bin/hiprt64.dll
ALL
)
endif()
```
Unless there are separate release and debug libraries, in which case:
```
if(EXISTS ${LIBDIR}/hiprt/bin/hiprt64.dll)
windows_install_shared_manifest(
FILES
${LIBDIR}/hiprt/bin/hiprt64.dll
RELEASE
)
windows_install_shared_manifest(
FILES
${LIBDIR}/hiprt/bin/hiprt64_d.dll
DEBUG
)
endif()
```
This is not going to be in Blender 4.2. Even if we get this landed soon, there is not enough time in the release cycle for this to get enough user testing and to fix potential issues. Bigger changes like this should land earlier.
I've been working on getting this to build on Linux, but ran into many issues. I pushed what I have so far but, there are still problems to resolve.
The premake build system is different from every other package we build, which makes integrating it harder. It would be much easier if this was using CMake as most other graphics software. Still it seems to work now on Linux, Windows not tested.
I had to patch the HIPRT code to fix some build issues. These have not been submitted upstream yet, because we should verify this works on Windows too.
I patched HIPRT with an option to disable unit tests. These use bundled version of Embree and TBB, but we don't want to use different versions than the rest of Blender.
It's not clear to me which build options should be enabled. There were a bunch listed in the original code, but which ones do we actually want to set, and how? Since the names don't correspond to premake options.
The repository requires submodules, which are not included in the release package. Either they should be included in the release upstream (I would prefer this), or we have to solve this better. There is currently a hack that downloads the git repository, but "Orochi" and "easy-encryption (why is this needed?) should become packages then.
Kernel compilation in Blender seems to take a long time. I have not timed yet, but it's been compiling for a single architecture for 10 minutes here and has not finished yet. I don't remember this happening before.
This is not going to be in Blender 4.2. Even if we get this landed soon, there is not enough time in the release cycle for this to get enough user testing and to fix potential issues. Bigger changes like this should land earlier.
I've been working on getting this to build on Linux, but ran into many issues. I pushed what I have so far but, there are still problems to resolve.
* The premake build system is different from every other package we build, which makes integrating it harder. It would be much easier if this was using CMake as most other graphics software. Still it seems to work now on Linux, Windows not tested.
* I had to patch the HIPRT code to fix some build issues. These have not been submitted upstream yet, because we should verify this works on Windows too.
* I patched HIPRT with an option to disable unit tests. These use bundled version of Embree and TBB, but we don't want to use different versions than the rest of Blender.
* It's not clear to me which build options should be enabled. There were a bunch listed in the original code, but which ones do we actually want to set, and how? Since the names don't correspond to premake options.
* The repository requires submodules, which are not included in the release package. Either they should be included in the release upstream (I would prefer this), or we have to solve this better. There is currently a hack that downloads the git repository, but "Orochi" and "easy-encryption (why is this needed?) should become packages then.
* Kernel compilation in Blender seems to take a long time. I have not timed yet, but it's been compiling for a single architecture for 10 minutes here and has not finished yet. I don't remember this happening before.
The repository requires submodules, which are not included in the release package
I ran into a similar issue when i was scripting manifold for the modelling module, ( #120712 ) I solved it there by downloading and unpacking the dep normally but setting the build command to echo "." and copying in the code in the patch step of the project that depends on it, not ideal, but likely the best we can do.
The changes you did seem like they make sense, but on windows this will require some more work since hiprt is using C++20 features (source_location) which isn't gonna be available on c++17 for msvc.
I gotta admit i'm a bit confused this got put up for review given the state it is in.
> The repository requires submodules, which are not included in the release package
I ran into a similar issue when i was scripting manifold for the modelling module, ( #120712 ) I solved it there by downloading and unpacking the dep normally but setting the build command to echo "." and copying in the code in the patch step of the project that depends on it, not ideal, but likely the best we can do.
The changes you did seem like they make sense, but on windows this will require some more work since hiprt is using C++20 features (source_location) which isn't gonna be available on c++17 for msvc.
I gotta admit i'm a bit confused this got put up for review given the state it is in.
It's not clear to me which build options should be enabled. There were a bunch listed in the original code, but which ones do we actually want to set, and how?
Let me check with HIPRT team and see why they haven't provided a CMake version yet. At any rate since you are calling Premake and passing bitcode option in hiprt.cmake the rest of the options are already taken care of.
HIPRT_EXPORTS and USE_HIP are already defined in Premake file by default.
bitcode --> HIPRT_BITCODE_LINKING and ORO_PRECOMPILED (Generating binaries for BVH kernel with the offline compiler)
bakeKernel --> HIPRT_LOAD_FROM_STRING (no longer necessary, mutually exclusive with above)
The repository requires submodules, which are not included in the release package. Either they should be included in the release upstream (I would prefer this), or we have to solve this better. There is currently a hack that downloads the git repository, but "Orochi" and "easy-encryption (why is this needed?) should become packages then.
Okay! Let me also get back to you on this and see if the team can do it in a cleaner way.
I don't think easy encryption is needed anymore; the code is open source and even then, kernel compilation is offline.
Kernel compilation in Blender seems to take a long time. I have not timed yet, but it's been compiling for a single architecture for 10 minutes here and has not finished yet. I don't remember this happening before.
I experienced something similar in the past with HIPRT 2.2 but not anymore with the latest compiler. What is your compiler version?
> It's not clear to me which build options should be enabled. There were a bunch listed in the original code, but which ones do we actually want to set, and how?
Let me check with HIPRT team and see why they haven't provided a CMake version yet. At any rate since you are calling Premake and passing bitcode option in [hiprt.cmake](https://projects.blender.org/salipour/AMD_HIPRT/src/branch/HIPRT_OPEN_SOURCE/build_files/build_environment/cmake/hiprt.cmake) the rest of the options are already taken care of.
HIPRT_EXPORTS and __USE_HIP__ are already defined in Premake file by default.
bitcode --> HIPRT_BITCODE_LINKING and ORO_PRECOMPILED (Generating binaries for BVH kernel with the offline compiler)
bakeKernel --> HIPRT_LOAD_FROM_STRING (no longer necessary, mutually exclusive with above)
> The repository requires submodules, which are not included in the release package. Either they should be included in the release upstream (I would prefer this), or we have to solve this better. There is currently a hack that downloads the git repository, but "Orochi" and "easy-encryption (why is this needed?) should become packages then.
Okay! Let me also get back to you on this and see if the team can do it in a cleaner way.
I don't think easy encryption is needed anymore; the code is open source and even then, kernel compilation is offline.
> Kernel compilation in Blender seems to take a long time. I have not timed yet, but it's been compiling for a single architecture for 10 minutes here and has not finished yet. I don't remember this happening before.
I experienced something similar in the past with HIPRT 2.2 but not anymore with the latest compiler. What is your compiler version?
I gotta admit i'm a bit confused this got put up for review given the state it is in.
Well, that was the advice, to quickly put something together and make a WIP pull request. Anyway, let me know whatever I can do to facilitate this process. Unfortunately, I stopped receiving notification whatsoever from projects.blender but will check the page regularly to not miss any communication.
> I gotta admit i'm a bit confused this got put up for review given the state it is in.
Well, that was the advice, to quickly put something together and make a WIP pull request. Anyway, let me know whatever I can do to facilitate this process. Unfortunately, I stopped receiving notification whatsoever from projects.blender but will check the page regularly to not miss any communication.
I made further changes to properly handle Orochi, disable encryption, some fixes for Windows (but still untested), fix library detection on Linux.
Next steps would be:
I would like to get the patches to HIPRT upstreamed. The way encryption was disabled is rather hacky. But I'm not sure if that code can just be removed entirely from the repo or if it's still needed for internal use at AMD and it's worth spending time to implement a proper way to turn it off/on. @salipour help with figuring this out would help. The patch set is included in the branch in build_files/build_environment/patches/hiprt.diff.
We'd like to be able to build HIPRT with C++17, following the VFX platform. In particular std::source_location is C++20.
I have not tried building this on Windows yet, it would help if @LazyDodo could try this. In theory it should work but there may well be some mistakes.
Further, a CMake based build would help though I guess now that we have it working it's not so important. Still if Linux distributions want to package hiprt for Blender, they probably would want a better solution than using a bundled premake binary, some solution to get the submodule, and an install target which properly install the files.
For the long build time, I was using an older compiler. With hipcc 6.1.1 it seems reasonable. Probably we should upgrade ROCm on the buildbots along with this.
I made further changes to properly handle Orochi, disable encryption, some fixes for Windows (but still untested), fix library detection on Linux.
Next steps would be:
* I would like to get the patches to HIPRT upstreamed. The way encryption was disabled is rather hacky. But I'm not sure if that code can just be removed entirely from the repo or if it's still needed for internal use at AMD and it's worth spending time to implement a proper way to turn it off/on. @salipour help with figuring this out would help. The patch set is included in the branch in `
build_files/build_environment/patches/hiprt.diff`.
* We'd like to be able to build HIPRT with C++17, following the VFX platform. In particular `std::source_location` is C++20.
* I have not tried building this on Windows yet, it would help if @LazyDodo could try this. In theory it should work but there may well be some mistakes.
Further, a CMake based build would help though I guess now that we have it working it's not so important. Still if Linux distributions want to package hiprt for Blender, they probably would want a better solution than using a bundled premake binary, some solution to get the submodule, and an install target which properly install the files.
For the long build time, I was using an older compiler. With hipcc 6.1.1 it seems reasonable. Probably we should upgrade ROCm on the buildbots along with this.
I forgot the C++20 issue. @salipour we could also use help to make HIPRT build with C++17. That's the version used by the VFX reference platform (https://vfxplatform.com/) that Blender and other software follows.
I forgot the C++20 issue. @salipour we could also use help to make HIPRT build with C++17. That's the version used by the VFX reference platform (https://vfxplatform.com/) that Blender and other software follows.
nothing too crazy there, next problems i haven't solved yet, i may get to it over the weekend, but i'll have to dive in to their build system a little bit.
Open problems:
is it relies on there being a system python since it just runs python somescript.py during the build , i'll have to see if i can fudge the path to include our python version.
C:\db\build\S\VS1564R\build\hiprt\src\external_hiprt>python tools/stringify.py ./hiprt/impl/Scene.h 20220318
1>>hiprt/cache/Kernels.h
Python was not found; run without arguments to install from the Microsoft Store, or disable this shortcut from
Settings > Manage App Execution Aliases.
the VS versions we use for the deps does not support C++20
c:\vs2019bt\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(636,5): error : Element <LanguageStandard> has
an invalid value of "stdcpp20". [C:\db\build\S\VS1564R\build\hiprt\src\external_hiprt\build\hiprt02003.vcxproj]
[c:\db\build\S\VS1564R\external_hiprt.vcxproj]
Fudging that in the premakefile to c++17, it tries to use source_location and naturally fails.
1>C:\db\build\S\VS1564R\build\hiprt\src\external_hiprt\hiprt\impl\Error.h(28,10): fatal error C1083: Cannot open include file: 'source_location': No such file or directory (compiling source file ..\hiprt\impl\BatchBuilder.cpp)
I won't have time today to fix these, but i'll try to make some time over the weekend.
i'd hold off on upstreaming these patches, there will be some work required for windows.
first off, the low hanging fruit
```
diff --git a/build_files/build_environment/cmake/hiprt.cmake b/build_files/build_environment/cmake/hiprt.cmake
index 01607a8c1fd..f9811a80a17 100644
--- a/build_files/build_environment/cmake/hiprt.cmake
+++ b/build_files/build_environment/cmake/hiprt.cmake
@@ -11,7 +11,7 @@ get_filename_component(_hip_path ${HIP_HIPCC_EXECUTABLE} DIRECTORY)
get_filename_component(_hip_path ${_hip_path} DIRECTORY)
if(WIN32)
- set(hiprt_configure HIP_PATH=${_hip_path} .\tools\premake5\win\premake5.exe vs2019)
+ set(hiprt_configure set HIP_PATH=${_hip_path} && .\\tools\\premake5\\win\\premake5.exe vs2019)
set(hiprt_build msbuild /m build/hiprt.sln /p:Configuration=Release)
else()
set(hiprt_configure HIP_PATH=${_hip_path} ./tools/premake5/linux64/premake5 gmake)
```
nothing too crazy there, next problems i haven't solved yet, i may get to it over the weekend, but i'll have to dive in to their build system a little bit.
Open problems:
- is it relies on there being a system python since it just runs `python somescript.py` during the build , i'll have to see if i can fudge the path to include our python version.
```plaintext
C:\db\build\S\VS1564R\build\hiprt\src\external_hiprt>python tools/stringify.py ./hiprt/impl/Scene.h 20220318
1>>hiprt/cache/Kernels.h
Python was not found; run without arguments to install from the Microsoft Store, or disable this shortcut from
Settings > Manage App Execution Aliases.
```
- the VS versions we use for the deps does _not_ support C++20
```plaintext
c:\vs2019bt\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(636,5): error : Element <LanguageStandard> has
an invalid value of "stdcpp20". [C:\db\build\S\VS1564R\build\hiprt\src\external_hiprt\build\hiprt02003.vcxproj]
[c:\db\build\S\VS1564R\external_hiprt.vcxproj]
```
- Fudging that in the premakefile to c++17, it tries to use source_location and naturally fails.
```plaintext
1>C:\db\build\S\VS1564R\build\hiprt\src\external_hiprt\hiprt\impl\Error.h(28,10): fatal error C1083: Cannot open include file: 'source_location': No such file or directory (compiling source file ..\hiprt\impl\BatchBuilder.cpp)
```
I won't have time today to fix these, but i'll try to make some time over the weekend.
I guess we can make the build system support providing a PYTHON_PATH. To implement that in the cleanest way possible I guess it would be best to replace the platform specific bat and bash files with a single Python script. I was a bit hesistant to start refactoring too much in HIPRT, but I guess it's not that difficult if we want to do that and upstream is open to it.
I guess we can make the build system support providing a `PYTHON_PATH`. To implement that in the cleanest way possible I guess it would be best to replace the platform specific bat and bash files with a single Python script. I was a bit hesistant to start refactoring too much in HIPRT, but I guess it's not that difficult if we want to do that and upstream is open to it.
Fixed most issues, and it's in a build-able state now, but some trivial things still remain i'll have to address
The trick of renaming the library to get rid of the version number in it, doesn't work on windows, there's always a .lib and .dll pair, and the .lib will have full name of the dll embedded in it, ie you can rename hiprt0200364.lib to hiprt.lib but the final binary that links it will always look for hiprt0200364.dll unsure what we want to do here. we can either go with the versioned names, or try to root out the version number wherever it is set.
The dll isn't being harvested yet
Fixed _most_ issues, and it's in a build-able state now, but some trivial things still remain i'll have to address
- The trick of renaming the library to get rid of the version number in it, doesn't work on windows, there's always a .lib and .dll pair, and the .lib will have full name of the dll embedded in it, ie you can rename `hiprt0200364.lib` to `hiprt.lib` but the final binary that links it will always look for `hiprt0200364.dll` unsure what we want to do here. we can either go with the versioned names, or try to root out the version number wherever it is set.
- The dll isn't being harvested yet
The trick of renaming the library to get rid of the version number in it, doesn't work on windows, there's always a .lib and .dll pair, and the .lib will have full name of the dll embedded in it, ie you can rename hiprt0200364.lib to hiprt.lib but the final binary that links it will always look for hiprt0200364.dll unsure what we want to do here. we can either go with the versioned names, or try to root out the version number wherever it is set.
We don't link against the stub library. HIP-RT dill is loaded dynamically. I've been doing this on Windows without problem
> - The trick of renaming the library to get rid of the version number in it, doesn't work on windows, there's always a .lib and .dll pair, and the .lib will have full name of the dll embedded in it, ie you can rename `hiprt0200364.lib` to `hiprt.lib` but the final binary that links it will always look for `hiprt0200364.dll` unsure what we want to do here. we can either go with the versioned names, or try to root out the version number wherever it is set.
We don't link against the stub library. HIP-RT dill is loaded dynamically. I've been doing this on Windows without problem
I forgot the C++20 issue. @salipour we could also use help to make HIPRT build with C++17. That's the version used by the VFX reference platform (https://vfxplatform.com/) that Blender and other software follows.
Sure, will do. Also, one of my colleagues is working to add CMake support and include the submodules in the release package.
> I forgot the C++20 issue. @salipour we could also use help to make HIPRT build with C++17. That's the version used by the VFX reference platform (https://vfxplatform.com/) that Blender and other software follows.
Sure, will do. Also, one of my colleagues is working to add CMake support and include the submodules in the release package.
Ah yes, technically we don't even need the .lib with the current implementation. We should add a comment about this though.
I wasn't sure if it would be best to patch hiprt to not include the version in the name (since that is quite non-standard), or if we needed the extra code complexity to deal with the version. But I guess we can do neither.
Ah yes, technically we don't even need the .lib with the current implementation. We should add a comment about this though.
I wasn't sure if it would be best to patch hiprt to not include the version in the name (since that is quite non-standard), or if we needed the extra code complexity to deal with the version. But I guess we can do neither.
yeah we can ignore the .lib if it's not being used, it was being harvested due to ${SHAREDLIBEXT} being .lib on windows, since for linkage we need to link the import library, and not the shared lib it self. For harvesting we just need to harvest the actual dll, and we can give it any name we want. The issue is a bit SHAREDLIBEXT is being used for something it wasn't meant for, maybe better to do something like this
Platform
SHAREDLIBEXT
SHAREDLIBIMPORTEXT
Windows
.dll
.lib
Mac
.dylib
.dylib
Linux
.so
.so
but that'll require changing quite a few files of the deps builder and with quite a few PR's in flight currently, i'd really rather not do that right now, for now think we can just hardcode the harvest to use .dll i'll get that done later this week.
yeah we can ignore the .lib if it's not being used, it was being harvested due to `${SHAREDLIBEXT}` being `.lib` on windows, since for linkage we need to link the import library, and not the shared lib it self. For harvesting we just need to harvest the actual dll, and we can give it any name we want. The issue is a bit `SHAREDLIBEXT` is being used for something it wasn't meant for, maybe better to do something like this
|Platform|SHAREDLIBEXT|SHAREDLIBIMPORTEXT
|-|-|-|
|Windows|`.dll`|`.lib`|
|Mac|`.dylib`|`.dylib`|
|Linux|`.so`|`.so`|
but that'll require changing quite a few files of the deps builder and with quite a few PR's in flight currently, i'd really rather not do that right now, for now think we can just hardcode the harvest to use .dll i'll get that done later this week.
Ah yes, technically we don't even need the .lib with the current implementation. We should add a comment about this though.
I wasn't sure if it would be best to patch hiprt to not include the version in the name (since that is quite non-standard), or if we needed the extra code complexity to deal with the version. But I guess we can do neither.
I think we can do away with hiprt version. The version is relevant, if the bitcode is supplied by the SDK.
> Ah yes, technically we don't even need the .lib with the current implementation. We should add a comment about this though.
>
> I wasn't sure if it would be best to patch hiprt to not include the version in the name (since that is quite non-standard), or if we needed the extra code complexity to deal with the version. But I guess we can do neither.
I think we can do away with hiprt version. The version is relevant, if the bitcode is supplied by the SDK.
@brecht
HIP-RT has been updated and now the changes are public: https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/releases
Let me know if there is any pending task from AMD side.
No way to set HIP_PATH on Linux. Also would be better as a cmake variable instead of environment variable.
No cmake option to build without encryption. (and easy-encryption is not in the release package, but if we build without we don't need it)
No cmake option to build without unit tests.
Assumes python command is available, but often there is only python3 on Linux.
There is no make install to install files.
Other issues found:
tools/bakeKernel.sh: 2: cannot create hiprt/cache/Kernels.h: Directory nonexistent. There is a wrong assumption about current working directory, it should be set explicitly in CMakeLists.txt.
@salipour It seems this added CMake support, but most of the reasons for why I needed these patches still remain. For references these are the patches:
https://projects.blender.org/blender/blender/src/commit/2ee2604f1f00bd1d931499715a1bbffb7906f84b/build_files/build_environment/patches/hiprt.diff
* No way to set `HIP_PATH` on Linux. Also would be better as a cmake variable instead of environment variable.
* No cmake option to build without encryption. (and `easy-encryption` is not in the release package, but if we build without we don't need it)
* No cmake option to build without unit tests.
* Assumes `python` command is available, but often there is only `python3` on Linux.
* There is no `make install` to install files.
Other issues found:
* `tools/bakeKernel.sh: 2: cannot create hiprt/cache/Kernels.h: Directory nonexistent`. There is a wrong assumption about current working directory, it should be set explicitly in `CMakeLists.txt`.
```
if(WIN32)
execute_process(COMMAND mkdir -p ${CMAKE_SOURCE_DIR}/hiprt/cache)
execute_process(COMMAND ${CMAKE_SOURCE_DIR}/tools/bakeKernel.bat WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
else()
execute_process(COMMAND mkdir -p ${CMAKE_SOURCE_DIR}/hiprt/cache)
execute_process(COMMAND ${CMAKE_SOURCE_DIR}/tools/bakeKernel.sh WORKING_DIRECTORY ${CMAKE_SOURCE_DIR})
endif()
```
easy-encription is no longer needed. Thanks for the feedback. We'll update HIP-RT and let you know.
Thanks. I got build error due to missing easy-encryption headers, and missing binary in the Python kernel baking code.
> easy-encription is no longer needed. Thanks for the feedback. We'll update HIP-RT and let you know.
Thanks. I got build error due to missing easy-encryption headers, and missing binary in the Python kernel baking code.
There is no make install to install files. @brecht did you mean make install in the hiprt project? From hiprt dist folder to blender lib folder? Or is it about cmake files in Blender?
> * There is no `make install` to install files.
@brecht did you mean make install in the hiprt project? From hiprt dist folder to blender lib folder? Or is it about cmake files in Blender?
I meant make install in the hiprt project. We can manually copy files from the dist folder too, it's not a big deal, just unusual for a cmake project not to have an install target where the target can be configured with CMAKE_INSTALL_PREFIX
I meant make install in the hiprt project. We can manually copy files from the dist folder too, it's not a big deal, just unusual for a cmake project not to have an install target where the target can be configured with `CMAKE_INSTALL_PREFIX`
you can build without easy-encryption (NO_ENCRYPT) but you probably want to leave easy encryption on until we fully test this option
you can build without unit test ( NO_UNITTEST )
you can specify the python command ( use env var: PYTHON_BIN )
there is a make install
Currently, Blender first looks for ROCM 5.x runtime (amdhip64.dll), then if the dll is not found, it checks ROCM 6.x (amdhip64_6.dll). HIP-RT order is the other way around. I still prefer to use the un-versioned dll since it's a tiny bit faster. In order to have HIP-RT load the un-versioned dll, you have to pass -DHIPRT_PREFER_HIP_5=ON as well.
example of command in Linux :
export PYTHON_BIN=python3
mkdir build
cmake -DCMAKE_BUILD_TYPE=Release -DBITCODE=ON -DNO_UNITTEST=ON -DHIP_PATH="/opt/rocm" -DPRECOMPILE=ON -DHIPRT_PREFER_HIP_5=ON -S . -B build
cmake --build build --config Release
cmake --install build
@brecht
My colleague, Richard, updated the HIP-RT repo and hopefully now the setup aligns with your expectations.
Here are the latest updates:
New release package:
https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/releases/tag/2.3.bd75b7c.rc4
cmake script updated:
- you can specify the HIP_PATH
- _**you can build without easy-encryption (NO_ENCRYPT) but you probably want to leave easy encryption on until we fully test this option**_
- you can build without unit test ( NO_UNITTEST )
- you can specify the python command ( use env var: PYTHON_BIN )
- there is a make install
Currently, Blender first looks for ROCM 5.x runtime (amdhip64.dll), then if the dll is not found, it checks ROCM 6.x (amdhip64_6.dll). HIP-RT order is the other way around. I still prefer to use the un-versioned dll since it's a tiny bit faster. In order to have HIP-RT load the un-versioned dll, you have to pass -DHIPRT_PREFER_HIP_5=ON as well.
example of command in Linux :
export PYTHON_BIN=python3
mkdir build
cmake -DCMAKE_BUILD_TYPE=Release -DBITCODE=ON -DNO_UNITTEST=ON -DHIP_PATH="/opt/rocm" -DPRECOMPILE=ON -DHIPRT_PREFER_HIP_5=ON -S . -B build
cmake --build build --config Release
cmake --install build
It's building ok now on Linux. Though I have not tested with an AMD GPU to check if it actually works.
I still had to work around two issues, but it could be done without patching so it's not too bad. Not sure if Windows will have a similar permission issue.
# Work around relative paths in bake kernel script and missing
# executable permission on encryption binary.
if(WIN32)
set(HIPRT_WORKAROUND
cd ${HIPRT_SOURCE_DIR} &&
)
else()
set(HIPRT_WORKAROUND
cd ${HIPRT_SOURCE_DIR} &&
chmod +x ./contrib/easy-encryption/bin/linux/ee64 &&
)
endif()
It's building ok now on Linux. Though I have not tested with an AMD GPU to check if it actually works.
I still had to work around two issues, but it could be done without patching so it's not too bad. Not sure if Windows will have a similar permission issue.
```
# Work around relative paths in bake kernel script and missing
# executable permission on encryption binary.
if(WIN32)
set(HIPRT_WORKAROUND
cd ${HIPRT_SOURCE_DIR} &&
)
else()
set(HIPRT_WORKAROUND
cd ${HIPRT_SOURCE_DIR} &&
chmod +x ./contrib/easy-encryption/bin/linux/ee64 &&
)
endif()
```
Brecht Van Lommel
requested review from Brecht Van Lommel 2024-06-20 17:03:14 +02:00
Brecht Van Lommel
requested review from Ray molenkamp 2024-06-20 17:03:27 +02:00
if(WIN32)
set(HIPRT_WORKAROUND
cd ${HIPRT_SOURCE_DIR} &&
)
else()
set(HIPRT_WORKAROUND
cd ${HIPRT_SOURCE_DIR} &&
chmod +x ./contrib/easy-encryption/bin/linux/ee64 &&
)
endif()
Yes, chmod +x ./contrib/easy-encryption/bin/linux/ee64 is needed because ee64 is used by the script during kernel pre-compilation.
> if(WIN32)
> set(HIPRT_WORKAROUND
> cd ${HIPRT_SOURCE_DIR} &&
> )
> else()
> set(HIPRT_WORKAROUND
> cd ${HIPRT_SOURCE_DIR} &&
> chmod +x ./contrib/easy-encryption/bin/linux/ee64 &&
> )
> endif()
> ```
Yes, chmod +x ./contrib/easy-encryption/bin/linux/ee64 is needed because ee64 is used by the script during kernel pre-compilation.
@LazyDodo@Sergey Please let me know if there is anything on AMD side that needs to be done before you can move forward. I really want to make sure this change is merged in time to pass all 4.3 check points. I still have to upload some changes for HIP-RT kernels to compile properly but want to wait until you sign off on the folder structure in lib folder for both Linux and Windows and HIP-RT binaries/headers are available for download on your repo.
@LazyDodo @Sergey Please let me know if there is anything on AMD side that needs to be done before you can move forward. I really want to make sure this change is merged in time to pass all 4.3 check points. I still have to upload some changes for HIP-RT kernels to compile properly but want to wait until you sign off on the folder structure in lib folder for both Linux and Windows and HIP-RT binaries/headers are available for download on your repo.
Sergey Sharybin
requested review from Sergey Sharybin 2024-07-10 10:02:08 +02:00
Sergey Sharybin
removed review request for Brecht Van Lommel 2024-07-10 10:02:13 +02:00
@salipour Hi. The folder structure seems fine to me. I wouldn't worry too much about it. It's easy to tweak at any time.
I think the plan here would be:
Update the patch against the latest main (let me know if it is something you can help us with)
Compile libraries for Linux and Windows, and commit those to branches in the pre-compiled repositories
Update this PR (or create a new one? Maybe Ray has strong feelings about it) to point to pre-compiled libraries with hiprt. And trigger a build.
Solve all issues we run into.
Land.
@salipour Hi. The folder structure seems fine to me. I wouldn't worry too much about it. It's easy to tweak at any time.
I think the plan here would be:
- Update the patch against the latest main (let me know if it is something you can help us with)
- Compile libraries for Linux and Windows, and commit those to branches in the pre-compiled repositories
- Update this PR (or create a new one? Maybe Ray has strong feelings about it) to point to pre-compiled libraries with hiprt. And trigger a build.
- Solve all issues we run into.
- Land.
@Sergey Can I upload the libraries to the repositories myself? I built the dependencies on Linux, but I wasn't able to do the same on Windows. @LazyDodo what do you think? Should I create a new pull request? The current pull request points to the hypothetical binaries/headers but it requires some refinement.
@Sergey Can I upload the libraries to the repositories myself? I built the dependencies on Linux, but I wasn't able to do the same on Windows.
@LazyDodo what do you think? Should I create a new pull request? The current pull request points to the hypothetical binaries/headers but it requires some refinement.
I Haven't looked at this in a while, so unsure about the exact state of this PR, I'll try to schedule in some time this week to take a closer look.
That being said what would a new PR bring to the table? Are there compelling reasons the issue cannot/should not be fixed in this PR?
I Haven't looked at this in a while, so unsure about the exact state of this PR, I'll try to schedule in some time this week to take a closer look.
That being said what would a new PR bring to the table? Are there compelling reasons the issue cannot/should not be fixed in this PR?
@LazyDodo There are some minor changes, nothing fundamental. And, the changes are related to the functionality of Blender build when it picks up the HIP-RT headers and binaries from third party lib folder. For example, in one of the make files, "orochi" is used, instead of "Orochi."
@LazyDodo There are some minor changes, nothing fundamental. And, the changes are related to the functionality of Blender build when it picks up the HIP-RT headers and binaries from third party lib folder. For example, in one of the make files, "orochi" is used, instead of "Orochi."
@salipour I am not really sure submitting libraries will speed things up here. We still need to ensure the build system properly compiles all libraries.
Did you compile those libraries on Rocky 8?
And for the Windows, is it you didn't have access to the platform yet, or are there some issues there?
@salipour I am not really sure submitting libraries will speed things up here. We still need to ensure the build system properly compiles all libraries.
Did you compile those libraries on Rocky 8?
And for the Windows, is it you didn't have access to the platform yet, or are there some issues there?
Ray molenkamp
requested changes 2024-07-11 17:33:07 +02:00
by setting justCONFIGURE_COMMAND and not BUILD_COMMAND cmake will just call make to build the project which is problematic since the PLATFORM_ALT_GENERATOR is set to ninja on windows.
by setting _just_ `CONFIGURE_COMMAND` and not `BUILD_COMMAND ` cmake will just call [`make` to build the project](https://cmake.org/cmake/help/latest/module/ExternalProject.html#build-step-options) which is problematic since the `PLATFORM_ALT_GENERATOR` is set to ninja on windows.
This doesn't work on windows, it requires set PYTHON_BIN=${PYTHON_BINARY} this probably be fixed by sticking it in the HIPRT_WORKAROUND section above however since that has been moved to a patch step and....
Further complications, hiprt just calls python and assumes it is in the path over here this still does not work on windows as there is still no system python. It be best if hiprt used the FindPython3 module to locate a suitable python interpreter and propagate the env var themselves to bakekernel.bat/sh or skip the batch file all together and just do the work from cmake.
With the env var now no longer being needed, and the workaround now being handled in the patch step, this whole section can now revert to using regular CMAKE_ARGS where cmake will call the right build tool on its own.
This doesn't work on windows, it requires `set PYTHON_BIN=${PYTHON_BINARY}` this probably be fixed by sticking it in the `HIPRT_WORKAROUND` section above however since that has been moved to a patch step and....
Further complications, hiprt just calls python and assumes it is in the path [over here](https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/blob/d14726ed77e26fe690ca8add17328d8f8cd2737f/CMakeLists.txt#L311) this still does not work on windows as there is still no system python. It be best if hiprt used the [FindPython3](https://cmake.org/cmake/help/latest/module/FindPython3.html) module to locate a suitable python interpreter and propagate the env var themselves to bakekernel.bat/sh or skip the batch file all together and just do the work from cmake.
With the env var now no longer being needed, and the workaround now being handled in the patch step, this whole section can now revert to using regular `CMAKE_ARGS` where cmake will call the right build tool on its own.
Further complications, hiprt just calls python and assumes it is in the path
Actually, PRECOMPILE=OFF will work fine. compile.py script generates the binaries and bitcodes. Now that HIP-RT is open-source, Cycles can directly build the binaries here
I will remove PRECOMPILE from hiprt.cmake. That should resolve all the complications about Python?
> Further complications, hiprt just calls python and assumes it is in the path
Actually, PRECOMPILE=OFF will work fine. compile.py script generates the binaries and bitcodes. Now that HIP-RT is open-source, Cycles can directly build the binaries [here ]([url](https://projects.blender.org/salipour/AMD_HIPRT/src/commit/8ea488273dd3186293377cb6acdec035e1f83805/intern/cycles/kernel/CMakeLists.txt#L798))
I will remove PRECOMPILE from hiprt.cmake. That should resolve all the complications about Python?
with BITCODE=OFF it appears to build fine with no further python issues, it warns about our cuda version
-- CUDA SDK install folder found: C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v12.3
The required version of CUDA for this Orochi is not found: 12.2. It's
advised that you install this version.
we're currently standardized on 12.3 but it doesn't appear to be causing build failures
with `BITCODE=OFF` it appears to build fine with no further python issues, it warns about our cuda version
```plaintext
-- CUDA SDK install folder found: C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v12.3
The required version of CUDA for this Orochi is not found: 12.2. It's
advised that you install this version.
```
we're currently standardized on 12.3 but it doesn't appear to be causing build failures
@salipour I am not really sure submitting libraries will speed things up here. We still need to ensure the build system properly compiles all libraries.
Did you compile those libraries on Rocky 8?
And for the Windows, is it you didn't have access to the platform yet, or are there some issues there?
I compiled all the libraries on Ubuntu 22.04. I had to install some other packages along the way, but nothing related to building HIP-RT. Should I try on Rocky? I haven't worked with it before, but I can try if that makes the process easier.
On Windows, which is my main dev environment, "make deps" is not recognized.
This is Brecht's response regarding building dependencies and open sourcing HIP-RT:
"It's not easy to run this libraries build script on Windows. It involves running build_files/build_environment/windows/build_deps.cmd. But it will take a long time and likely will fail in one of the many dependencies.
if you have Linux it would be easier to test there first, where there is a simpler "make deps" command that should work.
But it may be better to just quickly put something together, make a WIP pull request and let the platform maintainers resolve the issues. Having something helps make it clear which package, version and build options should be used."
I can start with build_deps.cmd and see how it goes.
> @salipour I am not really sure submitting libraries will speed things up here. We still need to ensure the build system properly compiles all libraries.
> Did you compile those libraries on Rocky 8?
> And for the Windows, is it you didn't have access to the platform yet, or are there some issues there?
@Sergey
I compiled all the libraries on Ubuntu 22.04. I had to install some other packages along the way, but nothing related to building HIP-RT. Should I try on Rocky? I haven't worked with it before, but I can try if that makes the process easier.
On Windows, which is my main dev environment, "make deps" is not recognized.
This is Brecht's response regarding building dependencies and open sourcing HIP-RT:
> "It's not easy to run this libraries build script on Windows. It involves running build_files/build_environment/windows/build_deps.cmd. But it will take a long time and likely will fail in one of the many dependencies.
>
> if you have Linux it would be easier to test there first, where there is a simpler "make deps" command that should work.
>
> But it may be better to just quickly put something together, make a WIP pull request and let the platform maintainers resolve the issues. Having something helps make it clear which package, version and build options should be used."
I can start with build_deps.cmd and see how it goes.
Windows and linux are fundamentally not all that different, except for the make helper, couple of pointers here though:
we build the deps on vs 2019 - v16.11.26 , it has seen no other more modern versions, you can try to use them, but this is untested, and unsupported.
Windows has Long path issues and things just break there,, my working folder for the deps builder is c:\db for that reason
a successful build of all deps takes about 12H on my environment, given hiprt is pretty standalone, you can likely skip most of them by just building the hiprt subproject, once build_deps.cmd prepares the build env and starts building the actual deps, kill it, and run the following batch file in c:\db
set MSSdk=1
set DISTUTILS_USE_SDK=1
set bld_org_dir=%~dp0
set TMPDIR=c:\t\
cd %bld_org_dir%\build\S\VS1564R\
cmake .
msbuild /m "external_hiprt.vcxproj" /p:Configuration=Release /fl /flp:logfile=BlenderDeps_hiprt.log;Verbosity=Diagnostic
cd %bld_org_dir%
to just build hiprt (and and its dependencies)
Windows and linux are fundamentally not all that different, except for the make helper, couple of pointers here though:
1) we build the deps on vs 2019 - v16.11.26 , it has seen no other more modern versions, you can try to use them, but this is untested, and unsupported.
2) Windows has Long path issues and things just break there,, my working folder for the deps builder is c:\db for that reason
3) a successful build of all deps takes about 12H on my environment, given hiprt is pretty standalone, you can likely skip most of them by just building the hiprt subproject, once build_deps.cmd prepares the build env and starts building the actual deps, kill it, and run the following batch file in `c:\db`
```
set MSSdk=1
set DISTUTILS_USE_SDK=1
set bld_org_dir=%~dp0
set TMPDIR=c:\t\
cd %bld_org_dir%\build\S\VS1564R\
cmake .
msbuild /m "external_hiprt.vcxproj" /p:Configuration=Release /fl /flp:logfile=BlenderDeps_hiprt.log;Verbosity=Diagnostic
cd %bld_org_dir%
```
to just build hiprt (and and its dependencies)
@LazyDodo Thank you for the instruction to build the dependencies on Windows; it was very helpful.
I can confirm HIP-RT builds successfully on Windows and Linux (Ubuntu.)
I still have to change BITCODE option back to ON (in hiprt.cmake) after HIP-RT changes make it to the public repo.
In the meantime, could you review the code and let me know if there were any concerns?
@LazyDodo Thank you for the instruction to build the dependencies on Windows; it was very helpful.
I can confirm HIP-RT builds successfully on Windows and Linux (Ubuntu.)
I still have to change BITCODE option back to ON (in hiprt.cmake) after HIP-RT changes make it to the public repo.
In the meantime, could you review the code and let me know if there were any concerns?
There seems to be something wrong went with the merge conflict resolution.
The harvesting part needs to be revisited, so it is aligned with what is doing #123196. At its current state the PR is not usable on Linux: there are a lot of CMake errors reported. If you can ensure it works on your side that'd definitely be of a great help and make it easier to move forward.
To replicate the Linux environment:
Install Rocky 8 in Minimal Install configuration.
Run sudo sh ./build_files/build_environment/linux/linux_rocky8_setup.sh.
I've verified the steps a few days ago with rev 471a148666, and the dependencies compiled correctly. Ideally the proposed PR would also work on this environment.
There seems to be something wrong went with the merge conflict resolution.
The harvesting part needs to be revisited, so it is aligned with what is doing #123196. At its current state the PR is not usable on Linux: there are a lot of CMake errors reported. If you can ensure it works on your side that'd definitely be of a great help and make it easier to move forward.
To replicate the Linux environment:
- Install Rocky 8 in `Minimal Install` configuration.
- Run `sudo sh ./build_files/build_environment/linux/linux_rocky8_setup.sh`.
- Add to the `~/.bash_profile`:
```
export LD_LIBRARY_PATH=/usr/local/cuda-12.5/lib64${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}
export PATH=/usr/local/cuda-12.5/bin${PATH:+:${PATH}}
```
- Clone blender.git, enable git-lfs libraries (`./build_files/utils/make_update.py --use-linux-libraries`).
- Run `make deps`.
I've verified the steps a few days ago with rev 471a1486660, and the dependencies compiled correctly. Ideally the proposed PR would also work on this environment.
My bad! I shouldn't have pushed the rebase before rebuilding dependencies. The harvest.cmake file, as you pointed out, was incorrectly merged. I fixed it and am building the dependencies on Ubuntu and Windows.
On another note, I had some issues with FindHIP module in finding HIP_LINKER_EXECUTABLE on Windows.
Without NO_DEFAULT_PATH the modules picks the clang++ from SYCL, so the flag has to stay. But with it, find_program() can't find the linker that is under HIP_PATH/bin.
If I limit the _hip_SEARCH_DIRS to only HIP_ROOT_DIR on Windows, then the linker is found. So, locally FindHIP looks like this for me
My bad! I shouldn't have pushed the rebase before rebuilding dependencies. The harvest.cmake file, as you pointed out, was incorrectly merged. I fixed it and am building the dependencies on Ubuntu and Windows.
On another note, I had some issues with FindHIP module in [finding ]([url](https://projects.blender.org/blender/blender/src/commit/a882473f3869317f0f5b3f94c853b5efe49f20e5/build_files/cmake/Modules/FindHIP.cmake#L41))HIP_LINKER_EXECUTABLE on Windows.
Without NO_DEFAULT_PATH the modules picks the clang++ from SYCL, so the flag has to stay. But with it, find_program() can't find the linker that is under HIP_PATH/bin.
If I limit the [_hip_SEARCH_DIRS ]([url](https://projects.blender.org/blender/blender/src/commit/a882473f3869317f0f5b3f94c853b5efe49f20e5/build_files/cmake/Modules/FindHIP.cmake#L24)) to only HIP_ROOT_DIR on Windows, then the linker is found. So, locally FindHIP looks like this for me
```
if(WIN32)
set(_hip_SEARCH_DIRS ${HIP_ROOT_DIR})
else()
set(_hip_SEARCH_DIRS
${HIP_ROOT_DIR}
/opt/rocm
/opt/rocm/hip)
endif()
```
EDIT: The links are not showing in the comment.
HIP_LINKER_EXECUTABLE : https://projects.blender.org/blender/blender/src/commit/a882473f3869317f0f5b3f94c853b5efe49f20e5/build_files/cmake/Modules/FindHIP.cmake#L41
_hip_SEARCH_DIRS :https://projects.blender.org/blender/blender/src/commit/a882473f3869317f0f5b3f94c853b5efe49f20e5/build_files/cmake/Modules/FindHIP.cmake#L24
Sahar A. Kashi
added 2 commits 2024-07-17 21:13:31 +02:00
@Sergey
I didn't run the dependency building to the completion but many of the dependencies, including hiprt, were successfully built by the time I ended the build.
I will try to build on Rocky as well; but at least Ubuntu seems to be in good shape.
@Sergey
I didn't run the dependency building to the completion but many of the dependencies, including hiprt, were successfully built by the time I ended the build.
I will try to build on Rocky as well; but at least Ubuntu seems to be in good shape.
@Sergey and @LazyDodo
I think everything is in good shape. I updated the version file to point to the latest version of HIP-RT that removes the Python dependency and confirm dependencies build is successful on Windows.
Rocky is still going on. But HIP-RT build is done and was successful.
Blender build with HIP-RT enabled is successful and functional. I have, however, experienced random crash with both HIP and HIP-RT and don't know yet whether it is related to rebasing to main or AMD driver or my system's instability.
Side note:
I had to disabled OpenVDB and projects that depended on it (USD and MaterialX.) OpenVDB required gcc version 9+ but after removing the entire tool set and reinstalling version 11 and running the rocky script, I would still get version 8.5 ¯(ツ)/¯
I also disabled LLVM build and all the projects that depend on it.
Please let me know the next step to finalize and complete this pull request.
EDIT: just in!
Dependencies successfully built and installed to /root/AMD_HIPRT/lib/linux_x64.
@Sergey and @LazyDodo
I think everything is in good shape. I updated the version file to point to the latest version of HIP-RT that removes the Python dependency and confirm dependencies build is successful on Windows.
Rocky is still going on. But HIP-RT build is done and was successful.
Blender build with HIP-RT enabled is successful and functional. I have, however, experienced random crash with both HIP and HIP-RT and don't know yet whether it is related to rebasing to main or AMD driver or my system's instability.
Side note:
I had to disabled OpenVDB and projects that depended on it (USD and MaterialX.) OpenVDB required gcc version 9+ but after removing the entire tool set and reinstalling version 11 and running the rocky script, I would still get version 8.5 ¯\(ツ)/¯
I also disabled LLVM build and all the projects that depend on it.
Please let me know the next step to finalize and complete this pull request.
EDIT: just in!
Dependencies successfully built and installed to /root/AMD_HIPRT/lib/linux_x64.
[root@localhost ~]# gcc --version
gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-22)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
[root@localhost ~]# scl enable gcc-toolset-11 bash
[root@localhost ~]# gcc --version
gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
did you activate the toolset?
```plaintext
[root@localhost ~]# gcc --version
gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-22)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
[root@localhost ~]# scl enable gcc-toolset-11 bash
[root@localhost ~]# gcc --version
gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
```
Thanks for the updates. I was able to compile hiprt library on Linux without a major issue. Currently testing the Blender build, which could take a bit. Meanwhile some comments based on reading the PR.
The version.txt was missing from install, not sure what's up with that. Maybe some harvest mask is wrong or something. I wouldn't worry about it, as the functionality of it needs to be taken care elsewhere. See the inlined comment about it.
There are also comments in some other areas of the code, which also needs to be addressed.
Thanks for the updates. I was able to compile hiprt library on Linux without a major issue. Currently testing the Blender build, which could take a bit. Meanwhile some comments based on reading the PR.
The `version.txt` was missing from install, not sure what's up with that. Maybe some harvest mask is wrong or something. I wouldn't worry about it, as the functionality of it needs to be taken care elsewhere. See the inlined comment about it.
There are also comments in some other areas of the code, which also needs to be addressed.
Not sure what's going on here. Looks fine in the branch on its ownhttps://projects.blender.org/salipour/AMD_HIPRT/src/branch/HIPRT_OPEN_SOURCE/build_files/build_environment/cmake/harvest.cmake
https://projects.blender.org/salipour/AMD_HIPRT/src/commit/4eb3743881df056ebaa1853ac5169f8361b84818/build_files/build_environment/cmake/harvest.cmake#L124
string_printf is a portable version of sprintf(), it doesn't print anything to the console. We should probably make it to warn if the result is unused to help catching issues like this.
Use LOG(ERROR) << "Failed to create hiprt Global Stack Buffer"; to report errors to the console.
`string_printf` is a portable version of `sprintf()`, it doesn't print anything to the console. We should probably make it to warn if the result is unused to help catching issues like this.
Use `LOG(ERROR) << "Failed to create hiprt Global Stack Buffer";` to report errors to the console.
For some reason version.txt was not copied to the lib/linux_x64/hiprt folder, so this part of the code couldn't have tested.
It also hardcodes compilation to a specific structure of percompiled libraries folder, making it harder to fit into Linux distros which ship Blender and don't use our precompiled libraries.
Such detection is to happen in FindHIPRT.cmake, based on the hiprt.h.
An example of this is SYCL_VERSION in FindSYCL.cmake, and OSL_LIBRARY_VERSION_MAJOR in FindOSL.cmake.
For some reason `version.txt` was not copied to the `lib/linux_x64/hiprt` folder, so this part of the code couldn't have tested.
It also hardcodes compilation to a specific structure of percompiled libraries folder, making it harder to fit into Linux distros which ship Blender and don't use our precompiled libraries.
Such detection is to happen in `FindHIPRT.cmake`, based on the `hiprt.h`.
An example of this is `SYCL_VERSION` in `FindSYCL.cmake`, and `OSL_LIBRARY_VERSION_MAJOR` in `FindOSL.cmake`.
It also hardcodes compilation to a specific structure of percompiled libraries folder, making it harder to fit into Linux distros which ship Blender and don't use our precompiled libraries.
@Sergey can you explain a bit more about how this doesn't fit nicely into Linux distribution? Does it mean that shaders are not compiled and will be compiled the first time Cycles is run?
> It also hardcodes compilation to a specific structure of percompiled libraries folder, making it harder to fit into Linux distros which ship Blender and don't use our precompiled libraries.
@Sergey can you explain a bit more about how this doesn't fit nicely into Linux distribution? Does it mean that shaders are not compiled and will be compiled the first time Cycles is run?
Many Linux distros are providing their own packages of Blender, with the dependencies from their repository, and do not use our pre-compiled libraries.
It is likely that Debian/Ubuntu/Gentoo will want to compile HIP-RT as a re-usable package, similarly how they deal with all the other dependencies (like, nvcc, oneapi/dpc++, libraries, etc). This means that the Blender build process can not rely on the way how some dependencies are apckaged.
Many Linux distros are providing their own packages of Blender, with the dependencies from their repository, and do not use our pre-compiled libraries.
It is likely that Debian/Ubuntu/Gentoo will want to compile HIP-RT as a re-usable package, similarly how they deal with all the other dependencies (like, nvcc, oneapi/dpc++, libraries, etc). This means that the Blender build process can not rely on the way how some dependencies are apckaged.
Why does this go to ${CMAKE_INSTALL_PREFIX}/blender.shared/${CMAKE_INSTALL_PREFIX}/lib and not to ${CYCLES_INSTALL_PATH}/lib ?
Using blender.shared here would break Cycles standalone.
Why does this go to `${CMAKE_INSTALL_PREFIX}/blender.shared`/`${CMAKE_INSTALL_PREFIX}/lib` and not to `${CYCLES_INSTALL_PATH}/lib` ?
Using `blender.shared` here would break Cycles standalone.
@Sergey is correct, but just as an educational opportunity for likely both, you can't install into blender.shared like that, it's governed by an manifest that keeps track what files are in that folder, if a shared lib is not listed in it, it will not be found by the OS loader. windows_install_shared_manifestMUST be used for all files installed there.
@Sergey is correct, but just as an educational opportunity for likely both, you can't install into `blender.shared` like that, it's governed by an manifest that keeps track what files are in that folder, if a shared lib is not listed in it, it will not be found by the OS loader. `windows_install_shared_manifest` _MUST_ be used for all files installed there.
@Sergey how should Cycles standalone be handled. The binaries should go wherever hiprt.dll goes. That also means for Linux without precompiled binaries, these binaries should get built and copied to where hiprt.so can see them. What do you recommend?
@Sergey how should Cycles standalone be handled. The binaries should go wherever hiprt.dll goes. That also means for Linux without precompiled binaries, these binaries should get built and copied to where hiprt.so can see them. What do you recommend?
@Sergey@LazyDodo
What do you think about the location of the couple binaries discussed here? They have to be seen by hiprt library. Cycle standalone? Linux first time compilation? Let me know what you think, and I can update the code accordingly.
@Sergey @LazyDodo
What do you think about the location of the couple binaries discussed here? They have to be seen by hiprt library. Cycle standalone? Linux first time compilation? Let me know what you think, and I can update the code accordingly.
Something that does not feel fully ideal, but matches what we do for oneAPI AoT kernels would work: https://projects.blender.org/blender/blender/src/commit/06fab9f8f1f2c23d7371c162875b1f374636ebf6/intern/cycles/kernel/CMakeLists.txt#L1144
As Ray was mentioning blender.shared should not be used directly.
oneAPI doesn't do it either. While it is a bit unideal to store libraries in the root of the application folder, it is known to work for now and for the ease of moving forward I'd suggest sticking to something we know is working for other backends.
As Ray was mentioning `blender.shared` should not be used directly.
oneAPI doesn't do it either. While it is a bit unideal to store libraries in the root of the application folder, it is known to work for now and for the ease of moving forward I'd suggest sticking to something we know is working for other backends.
This is blender standalone specific, for mainline blender, yes, it be un-ideal to have more shared libs next to blender than required, (it's the main reason blender.shared exists, people complained there was too much "stuff" in the main blender folder) for standalone, for me personally at least that's not a concern, right by the binary is fine to me.
This is blender standalone specific, for mainline blender, yes, it be un-ideal to have more shared libs next to blender than required, (it's the main reason blender.shared exists, people complained there was too much "stuff" in the main blender folder) for standalone, for me personally at least that's not a concern, right by the binary is fine to me.
Right now, this is how it is laid out:
Cycles stand-alone: hiprt64.dll and shader binaries (hipfb files) are in the root directory
Blender full build: hiprt64.dll and shader binaries are in the blender.shared folder.
Is it acceptable as is? Or do you prefer moving the files to the root directory for Blender full build as well?
Since shader binaries are loaded by hiprt64.dll and not by the OS loader, is it okay not to use windows_install_shared_manifest?
And, for possible future improvement, what will an ideal solution look like?
Right now, this is how it is laid out:
Cycles stand-alone: hiprt64.dll and shader binaries (hipfb files) are in the root directory
Blender full build: hiprt64.dll and shader binaries are in the blender.shared folder.
Is it acceptable as is? Or do you prefer moving the files to the root directory for Blender full build as well?
Since shader binaries are loaded by hiprt64.dll and not by the OS loader, is it okay not to use windows_install_shared_manifest?
And, for possible future improvement, what will an ideal solution look like?
There is a bit of a difference between the final layout, and where/how CMake rules are written for this. There also seems to be a level of confusion about precompiled libraries, and the BVH libraries.
The hiprt64.dll is to be installed into blender.shared using windows_install_shared_manifest. This is already handled in the source/creator/CMakeLists.txt in this version of PR. It is probably fine, although not fully clear when something is to be handled via explicitly there, and what is to go via PLATFORM_BUNDLED_LIBRARIES. The latter one seems more modern way of how we handle bundling, but hearing from Ray would be good.
In any case no further handling of blender.shared is to happen from Cycles.
On linux the hiprt.so is handled by add_bundled_libraries(hiprt/lib) in platform_unix.cmake, so that's also already taken care of.
For the Cycles standalone bundling of such libraries is done by a CMake rules which is not currently in the main Blender repo. That we'd need to take care of when we'll be porting changes to the standalone repository. It is not something that seems possible to handle as this PR.
The installation of ${bvh_file} and ${bvh_file_oro} (which, I believe, part of the HWRT that is compiled from kernel sources) is to follow the same rules as current cycles_kernel_oneapi_lib (which is the closest similar thing we already do with the other backend). I wrote details about it in the previous comment.
There is a bit of a difference between the final layout, and where/how CMake rules are written for this. There also seems to be a level of confusion about precompiled libraries, and the BVH libraries.
The `hiprt64.dll` is to be installed into `blender.shared` using `windows_install_shared_manifest`. This is already handled in the `source/creator/CMakeLists.txt` in this version of PR. It is probably fine, although not fully clear when something is to be handled via explicitly there, and what is to go via `PLATFORM_BUNDLED_LIBRARIES`. The latter one seems more modern way of how we handle bundling, but hearing from Ray would be good.
In any case no further handling of `blender.shared` is to happen from Cycles.
On linux the `hiprt.so` is handled by `add_bundled_libraries(hiprt/lib)` in `platform_unix.cmake`, so that's also already taken care of.
For the Cycles standalone bundling of such libraries is done by a CMake rules which is not currently in the main Blender repo. That we'd need to take care of when we'll be porting changes to the standalone repository. It is not something that seems possible to handle as this PR.
The installation of `${bvh_file}` and `${bvh_file_oro}` (which, I believe, part of the HWRT that is compiled from kernel sources) is to follow the same rules as current `cycles_kernel_oneapi_lib` (which is the closest similar thing we already do with the other backend). I wrote details about it in the previous comment.
although not fully clear when something is to be handled via explicitly there, and what is to go via PLATFORM_BUNDLED_LIBRARIES. The latter one seems more modern way of how we handle bundling, but hearing from Ray would be good.
afaik PLATFORM_BUNDLED_LIBRARIES isn't used on windows, it lacks the functionality to install different things depending on debug/release builds, we could likely make it work with some macro magic but as of right now this is not the case and isn't used (that i'm aware of)
> although not fully clear when something is to be handled via explicitly there, and what is to go via PLATFORM_BUNDLED_LIBRARIES. The latter one seems more modern way of how we handle bundling, but hearing from Ray would be good.
afaik `PLATFORM_BUNDLED_LIBRARIES` isn't used on windows, it lacks the functionality to install different things depending on debug/release builds, we could likely make it work with some macro magic but as of right now this is not the case and isn't used (that i'm aware of)
Sahar A. Kashi
added 2 commits 2024-07-24 22:52:20 +02:00
Building dependencies are successful, so that's good. Although, is seemingly incomplete.
Building Blender with HIP-RT enabled fails compilation:
make[2]: *** No rule to make target '/home/sergey/Developer/bf/blender/lib/linux_x64/hiprt/include/hiprt/impl/hiprt_kernels_bitcode.h', needed by 'intern/cycles/kernel/hiprt02003_6.1_amd_lib.bc'. Stop.
Either something is missing in build configuration, or harvesting is missing.
If you start with a clean Linux environment (as described in some earlier comment) and clean checkout, this should work:
Apply patch against the latest main branch
make deps
make CMAKE_CONFIG_ARGS='-DWITH_CYCLES_DEVICE_HIPRT=ON' release
You shouldn't be needing to copy any files manually to the libraries folder. It should be handled by the dependency builder (during the make deps step).
Installation part
For the installation steps we can either simply copy the same logic as is done around delayed_install("" "${cycles_kernel_oneapi_lib}" ${cycles_oneapi_target_path}) (including the cycles_oneapi_target_path part). Or, more ideally, move the if(NOT WITH_BLENDER) closer to the beginning of the file (making it reusable), and have
if(NOT WITH_BLENDER)
# For the Cycles standalone put libraries next to the Cycles application.
set(cycles_kernel_runtime_lib_target_path ${CYCLES_INSTALL_PATH})
else()
# For Blender put the libraries next to the Blender executable.
#
# Note that the installation path in the delayed_install is relative to the versioned folder,
# which means we need to go one level up.
set(cycles_kernel_runtime_lib_target_path "../")
endif()
if(UNIX AND NOT APPLE)
set(cycles_kernel_runtime_lib_target_path ${cycles_kernel_runtime_lib_target_path}/lib)
endif()
And then oneAPI case becomes: delayed_install("" "${cycles_kernel_runtime_lib_target_path}" ${cycles_kernel_runtime_lib_target_path})
And the HIP-RT:
### Compilation part
Building dependencies are successful, so that's good. Although, is seemingly incomplete.
Building Blender with HIP-RT enabled fails compilation:
```
make[2]: *** No rule to make target '/home/sergey/Developer/bf/blender/lib/linux_x64/hiprt/include/hiprt/impl/hiprt_kernels_bitcode.h', needed by 'intern/cycles/kernel/hiprt02003_6.1_amd_lib.bc'. Stop.
```
Either something is missing in build configuration, or harvesting is missing.
If you start with a clean Linux environment (as described in some earlier comment) and clean checkout, this should work:
- Apply patch against the latest `main branch`
- `make deps`
- `make CMAKE_CONFIG_ARGS='-DWITH_CYCLES_DEVICE_HIPRT=ON' release`
You shouldn't be needing to copy any files manually to the libraries folder. It should be handled by the dependency builder (during the `make deps` step).
### Installation part
For the installation steps we can either simply copy the same logic as is done around `delayed_install("" "${cycles_kernel_oneapi_lib}" ${cycles_oneapi_target_path})` (including the `cycles_oneapi_target_path` part). Or, more ideally, move the `if(NOT WITH_BLENDER)` closer to the beginning of the file (making it reusable), and have
```
if(NOT WITH_BLENDER)
# For the Cycles standalone put libraries next to the Cycles application.
set(cycles_kernel_runtime_lib_target_path ${CYCLES_INSTALL_PATH})
else()
# For Blender put the libraries next to the Blender executable.
#
# Note that the installation path in the delayed_install is relative to the versioned folder,
# which means we need to go one level up.
set(cycles_kernel_runtime_lib_target_path "../")
endif()
if(UNIX AND NOT APPLE)
set(cycles_kernel_runtime_lib_target_path ${cycles_kernel_runtime_lib_target_path}/lib)
endif()
```
And then oneAPI case becomes: `delayed_install("" "${cycles_kernel_runtime_lib_target_path}" ${cycles_kernel_runtime_lib_target_path})`
And the HIP-RT:
```
delayed_install("" "${bvh_file}" ${cycles_kernel_runtime_lib_target_path})
delayed_install("" "${bvh_file_oro}" ${cycles_kernel_runtime_lib_target_path})
```
Precompiled libraries for Cycles standalone are handled outside of this file. There should be no need to have this special case here.
We'd need to handle it in the external_libs.cmake of the standalone repo when we'll be porting the change there.
Precompiled libraries for Cycles standalone are handled outside of this file. There should be no need to have this special case here.
We'd need to handle it in the `external_libs.cmake` of the standalone repo when we'll be porting the change there.
I am not sure where to move this part. I was falling back to the same logic as One API: https://projects.blender.org/blender/blender/src/commit/3acb5ae41b7c0eaf16e08786a1f15a95f202dbbb/intern/cycles/kernel/CMakeLists.txt#L864
Also, I guess I have to remove this part. https://projects.blender.org/salipour/AMD_HIPRT/src/commit/a73c214129e3aab7279e1e57120df6971a0f4a7d/source/creator/CMakeLists.txt#L1835
@Sergey were you referring to this https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/cmake/external_libs.cmake
There are just few extra things that happen here and seeming should not happen.
From the fact that bvh_file and bvh_file_oro were initially targeted to be installed to a libs folder I've assumed they are dynamic libraries and should be handled something like this:
# TODO: De-duplicate path logic with oneAPI case.
if(NOT WITH_BLENDER)
# For the Cycles standalone put libraries next to the Cycles application.
set(cycles_hiprt_target_path ${CYCLES_INSTALL_PATH})
else()
# For Blender put the libraries next to the Blender executable.
#
# Note that the installation path in the delayed_install is relative to the versioned folder,
# which means we need to go one level up.
set(cycles_hiprt_target_path "../")
endif()
if(UNIX AND NOT APPLE)
set(cycles_hiprt_target_path "${cycles_hiprt_target_path}/lib")
endif()
# install dynamic libraries required at runtime
delayed_install("" "${bvh_file}" ${cycles_hiprt_target_path})
delayed_install("" "${bvh_file_oro}" ${cycles_hiprt_target_path})
However, they are not a dynamic library, but a data of some sort. So from this perspective they seem to belong to the same place as the rest of the precompiled kernels (cubins, fatbins, and the kernel_rt_gfx.hipfb.zst).
Where exactly those these bvh_file and bvh_file_oro (hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb) are used during runtime?
> I am not sure where to move this part
There are just few extra things that happen here and seeming should not happen.
From the fact that `bvh_file` and `bvh_file_oro` were initially targeted to be installed to a `libs` folder I've assumed they are dynamic libraries and should be handled something like this:
```
# TODO: De-duplicate path logic with oneAPI case.
if(NOT WITH_BLENDER)
# For the Cycles standalone put libraries next to the Cycles application.
set(cycles_hiprt_target_path ${CYCLES_INSTALL_PATH})
else()
# For Blender put the libraries next to the Blender executable.
#
# Note that the installation path in the delayed_install is relative to the versioned folder,
# which means we need to go one level up.
set(cycles_hiprt_target_path "../")
endif()
if(UNIX AND NOT APPLE)
set(cycles_hiprt_target_path "${cycles_hiprt_target_path}/lib")
endif()
# install dynamic libraries required at runtime
delayed_install("" "${bvh_file}" ${cycles_hiprt_target_path})
delayed_install("" "${bvh_file_oro}" ${cycles_hiprt_target_path})
```
However, they are not a dynamic library, but a data of some sort. So from this perspective they seem to belong to the same place as the rest of the precompiled kernels (cubins, fatbins, and the `kernel_rt_gfx.hipfb.zst`).
Where exactly those these `bvh_file` and `bvh_file_oro` (`hiprt02003_6.1_amd.hipfb` and `oro_compiled_kernels.hipfb`) are used during runtime?
bvh_file and bvh_file_oro are loaded by hiprt64.dll. If moved to where rest of the precompiled kernels are, hiprt64 will not be able to load them.
Here is when they are used:
bvh_file and bvh_file_oro are loaded by hiprt64.dll. If moved to where rest of the precompiled kernels are, hiprt64 will not be able to load them.
Here is when they are used:
https://projects.blender.org/salipour/AMD_HIPRT/src/commit/06076d74b1bec0883bfd63afd830a91acf254882/intern/cycles/device/hiprt/device_impl.cpp#L788
and
https://projects.blender.org/salipour/AMD_HIPRT/src/commit/06076d74b1bec0883bfd63afd830a91acf254882/intern/cycles/device/hiprt/device_impl.cpp#L994
If you start with a clean Linux environment (as described in some earlier comment) and clean checkout, this should work:
Apply patch against the latest main branch
make deps
make CMAKE_CONFIG_ARGS='-DWITH_CYCLES_DEVICE_HIPRT=ON' release
No WITH_CYCLES_HIP_BINARIES?
> If you start with a clean Linux environment (as described in some earlier comment) and clean checkout, this should work:
> - Apply patch against the latest `main branch`
> - `make deps`
> - `make CMAKE_CONFIG_ARGS='-DWITH_CYCLES_DEVICE_HIPRT=ON' release`
No WITH_CYCLES_HIP_BINARIES?
The WITH_CYCLES_HIP_BINARIES are taken care by the release configuration (in the blender_release.cmake).
The issue can be seen before building Blender: the make deps does not place the hiprt_kernels_bitcode.h anywhere in the lib/linux_x64/hiprt. The hiprt libraries and some headers are copied there, bit the hiprt_kernels_bitcode.h is missing.
This is something me and Ray quickly talked about: it seems on Windows files are not copied automatically into the final location, while they do seem to copy on macOS and Linux. In any case, the hiprt_kernels_bitcode.h is not found in neither under ./lib/linux_x64/hiprt submodule, nor in build_linux/deps_x64/Release/hiprt.
So some harvesting or copying seems to be missing.
> No `WITH_CYCLES_HIP_BINARIES`?
The `WITH_CYCLES_HIP_BINARIES` are taken care by the release configuration (in the `blender_release.cmake`).
The issue can be seen before building Blender: the `make deps` does not place the `hiprt_kernels_bitcode.h` anywhere in the `lib/linux_x64/hiprt`. The hiprt libraries and some headers are copied there, bit the `hiprt_kernels_bitcode.h` is missing.
This is something me and Ray quickly talked about: it seems on Windows files are not copied automatically into the final location, while they do seem to copy on macOS and Linux. In any case, the `hiprt_kernels_bitcode.h` is not found in neither under `./lib/linux_x64/hiprt` submodule, nor in `build_linux/deps_x64/Release/hiprt`.
So some harvesting or copying seems to be missing.
The WITH_CYCLES_HIP_BINARIES are taken care by the release configuration (in the blender_release.cmake).
The issue can be seen before building Blender: the make deps does not place the hiprt_kernels_bitcode.h anywhere in the lib/linux_x64/hiprt. The hiprt libraries and some headers are copied there, bit the hiprt_kernels_bitcode.h is missing.
This is something me and Ray quickly talked about: it seems on Windows files are not copied automatically into the final location, while they do seem to copy on macOS and Linux. In any case, the hiprt_kernels_bitcode.h is not found in neither under ./lib/linux_x64/hiprt submodule, nor in build_linux/deps_x64/Release/hiprt.
So some harvesting or copying seems to be missing.
Yea, it has to do with the harvesting. I had tested HIP-RT build on Ubuntu before but not after #123196.
> > No `WITH_CYCLES_HIP_BINARIES`?
>
> The `WITH_CYCLES_HIP_BINARIES` are taken care by the release configuration (in the `blender_release.cmake`).
>
> The issue can be seen before building Blender: the `make deps` does not place the `hiprt_kernels_bitcode.h` anywhere in the `lib/linux_x64/hiprt`. The hiprt libraries and some headers are copied there, bit the `hiprt_kernels_bitcode.h` is missing.
>
> This is something me and Ray quickly talked about: it seems on Windows files are not copied automatically into the final location, while they do seem to copy on macOS and Linux. In any case, the `hiprt_kernels_bitcode.h` is not found in neither under `./lib/linux_x64/hiprt` submodule, nor in `build_linux/deps_x64/Release/hiprt`.
> So some harvesting or copying seems to be missing.
Yea, it has to do with the harvesting. I had tested HIP-RT build on Ubuntu before but not after #123196.
Sahar A. Kashi
added 1 commit 2024-08-08 04:48:23 +02:00
Actually, today I've noticed something strange: the release part of the make command did not properly pick up the release CMake configuration.
It is nothing to do with this PR, and I didn't see it happening before. So we'd need to keep an eye when testing things with such one-liners.
Yea, it has to do with the harvesting. I had tested HIP-RT build on Ubuntu before but not after #123196.
Ah, cool. The updated harvest.cmake seems to do the proper thing now.
Think I will leave the make deps part of the review up to the maintainers at this pont. Its their domain of expertise.
I've merged the latest main into the branch, and solved some various cosmetic issues. General notes for the future:
It is a good idea to review own code after submission, to ensure PR contains everything you expect, and doesn't contain anything you don't expect.
Avoid doing unnecessary whitespace changes and other cleanups.
Dead code is discouraged.
Make sure clang-format and autopep8 is configured and code follows our style guidelines.
The things that need attention from my side:
The bvh_file and bvh_file_oro (hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb). They do not seem to belong to the libs folder, but it is also unclear where they are used form.
The environment variable manipulation seems unnecessary and fragile/non-portable.
Maybe something else i forgot, follow the inlined comments.
> No `WITH_CYCLES_HIP_BINARIES`?
Actually, today I've noticed something strange: the `release` part of the `make` command did not properly pick up the release CMake configuration.
It is nothing to do with this PR, and I didn't see it happening before. So we'd need to keep an eye when testing things with such one-liners.
> Yea, it has to do with the harvesting. I had tested HIP-RT build on Ubuntu before but not after #123196.
Ah, cool. The updated `harvest.cmake` seems to do the proper thing now.
Think I will leave the `make deps` part of the review up to the maintainers at this pont. Its their domain of expertise.
I've merged the latest `main` into the branch, and solved some various cosmetic issues. General notes for the future:
- It is a good idea to review own code after submission, to ensure PR contains everything you expect, and doesn't contain anything you don't expect.
- Avoid doing unnecessary whitespace changes and other cleanups.
- Dead code is discouraged.
- Make sure clang-format and autopep8 is configured and code follows our style guidelines.
The things that need attention from my side:
- The `bvh_file` and `bvh_file_oro` (`hiprt02003_6.1_amd.hipfb` and `oro_compiled_kernels.hipfb`). They do not seem to belong to the libs folder, but it is also unclear where they are used form.
- The environment variable manipulation seems unnecessary and fragile/non-portable.
- Maybe something else i forgot, follow the inlined comments.
Hey sorry didn't mean for this change list to get reviewed. I was moving from Linux to Windows so, I pushed the changes to pick up on Windows. Should have pushed to a different branch!
Hey sorry didn't mean for this change list to get reviewed. I was moving from Linux to Windows so, I pushed the changes to pick up on Windows. Should have pushed to a different branch!
I don't really understand where this is uses. The only access to the HIPRT_PATH environment variable I see is few lines below. What parts of code actually depend on it?
If it's only used here in the device, no need to set an environment variable.
If it's somehow used by other parts of our code then:
Add a comment where it is used.
Consider having a different name. HIPRT_PATH sounds like a path to HIP-RT SDK, and not to our kernel code.
I don't really understand where this is uses. The only access to the `HIPRT_PATH` environment variable I see is few lines below. What parts of code actually depend on it?
If it's only used here in the device, no need to set an environment variable.
If it's somehow used by other parts of our code then:
- Add a comment where it is used.
- Consider having a different name. `HIPRT_PATH` sounds like a path to HIP-RT SDK, and not to our kernel code.
@Sergey sorry didn't mean for this change list to get reviewed. I was moving from Linux to Windows so I pushed the changes to pick up on Windows. Should have pushed to a different branch!
That was actually the idea. To set HIPRT_PATH if HIPRT_SDK wasn't already set.
I was trying to move compilation of bvh_file and bvh_file_oro to first time the app is launched, since these two files and when they are placed have been the subject of a lot of discussion.
The way it works inside hiprt library is that hiprt looks for bvh_file and bvh_file_oro and if they don't exist, it compiled them from source.
There are a number of issues with that approach:
1- The end user should have HIP SDK/compiler installed for that to work
2- This part is supposed to happen under the hood and an application using the library doesnt need to know about what is happening, so it shouldn't have to set the PATH either
3- Plus, even if that was an acceptable solution, the environment variable should get set before hiprt64 is loaded, so not in this section of the code.
I admit the limitation where to place bvh_file and bvh_file_oro is not ideal and borderline breaks the patterns in Blender but right now putting hiprt64 and the two bvh shader binaries in the same location (e.g. root directory) is the only option I can think of.
@Sergey sorry didn't mean for this change list to get reviewed. I was moving from Linux to Windows so I pushed the changes to pick up on Windows. Should have pushed to a different branch!
That was actually the idea. To set HIPRT_PATH if HIPRT_SDK wasn't already set.
I was trying to move compilation of bvh_file and bvh_file_oro to first time the app is launched, since these two files and when they are placed have been the subject of a lot of discussion.
The way it works inside hiprt library is that hiprt looks for bvh_file and bvh_file_oro and if they don't exist, it compiled them from source.
There are a number of issues with that approach:
1- The end user should have HIP SDK/compiler installed for that to work
2- This part is supposed to happen under the hood and an application using the library doesnt need to know about what is happening, so it shouldn't have to set the PATH either
3- Plus, even if that was an acceptable solution, the environment variable should get set before hiprt64 is loaded, so not in this section of the code.
I admit the limitation where to place bvh_file and bvh_file_oro is not ideal and borderline breaks the patterns in Blender but right now putting hiprt64 and the two bvh shader binaries in the same location (e.g. root directory) is the only option I can think of.
@Sergey what do you think about the placement of these files? I have them all in the root directory. I have built the dependencies and the app on both Windows and Ubuntu. It is functional with the current version of the code.
@Sergey what do you think about the placement of these files? I have them all in the root directory. I have built the dependencies and the app on both Windows and Ubuntu. It is functional with the current version of the code.
There is no need to get an environment variable right after it was set. Just use the std::string variable from above (the hiprt_path_runtime but without HIPRT_PATH= part).
There is no need to get an environment variable right after it was set. Just use the `std::string` variable from above (the `hiprt_path_runtime` but without `HIPRT_PATH=` part).
salipour marked this conversation as resolved
Sahar A. Kashi
added 2 commits 2024-08-09 20:38:03 +02:00
Updated the launch time kernel compilation to match the current hiprt kernel layout
Moved bvh kernel binary compilation to build time regardless of WITH_CYCLES_HIP_BINARIES value
@salipour Can you write a small design summary, making it more clear what are the moving parts, and when things are supposed to happen (AoT, JIT, etc)? It is not something that is 100% clean, and things seem to go back-n-forth in the code, which does not help understanding what's going on.
On top of that, it would be very nice if the updates to the PR were more descriptive. Some of the recent commits seem to include unrelated changes. For example, the Updated the copy location of bvh kernel binaries includes changes to the way how the library is built (-DBITCODE=ON), without mentioning this.
Keeping in mind previous conversation about this makes it extra unclear: some messages seem to suggest bitcode is not needed, but then that it actually is needed because of runtime, but also there are some compilation errors mentioned related to Python and BITCODE. It is very confusing.
I admit the limitation where to place bvh_file and bvh_file_oro is not ideal and borderline breaks the patterns in Blender but right now putting hiprt64 and the two bvh shader binaries in the same location (e.g. root directory) is the only option I can think of.
I don't really understand why those files could not provided explicitly via API. Similarly how render kernel does not need to follow any naming convention, and does not need to be in any specific location w.r.t to driver or other libraries.
@salipour Can you write a small design summary, making it more clear what are the moving parts, and when things are supposed to happen (AoT, JIT, etc)? It is not something that is 100% clean, and things seem to go back-n-forth in the code, which does not help understanding what's going on.
On top of that, it would be very nice if the updates to the PR were more descriptive. Some of the recent commits seem to include unrelated changes. For example, the `Updated the copy location of bvh kernel binaries` includes changes to the way how the library is built (`-DBITCODE=ON`), without mentioning this.
Keeping in mind previous conversation about this makes it extra unclear: some messages seem to suggest bitcode is not needed, but then that it actually is needed because of runtime, but also there are some compilation errors mentioned related to Python and BITCODE. It is very confusing.
> I admit the limitation where to place bvh_file and bvh_file_oro is not ideal and borderline breaks the patterns in Blender but right now putting hiprt64 and the two bvh shader binaries in the same location (e.g. root directory) is the only option I can think of.
I don't really understand why those files could not provided explicitly via API. Similarly how render kernel does not need to follow any naming convention, and does not need to be in any specific location w.r.t to driver or other libraries.
Please remove handling of libhiprt64.so and hiprt64.dll from this file.
For the purpose of this PR, Linux is handled via PLATFORM_BUNDLED_LIBRARIES (hiprt is added via add_bundled_library(hiprt/lib)). Windows is to be handled by the CMakeLists.txt. Some recent change removed that code.
The Cycles standalone we'll handle when we'll be porting the changes there. The important thing here is to just write code in a way that will not cause problems for that porting process. If you follow what I've write then updating the standalone repository will be easy.
Please remove handling of `libhiprt64.so` and `hiprt64.dll` from this file.
For the purpose of this PR, Linux is handled via `PLATFORM_BUNDLED_LIBRARIES` (hiprt is added via `add_bundled_library(hiprt/lib)`). Windows is to be handled by the `CMakeLists.txt`. Some recent change removed that code.
The Cycles standalone we'll handle when we'll be porting the changes there. The important thing here is to just write code in a way that will not cause problems for that porting process. If you follow what I've write then updating the standalone repository will be easy.
@Sergey
The explanations for the different parts are scattered across various threads, so I'll do my best to consolidate everything here.
Please remove handling of libhiprt64.so and hiprt64.dll from this file.
Will do. I added it temporarily until the kernel binaries placement is sorted out (more on this at the end of the comments)
For the purpose of this PR, Linux is handled via PLATFORM_BUNDLED_LIBRARIES (hiprt is added via add_bundled_library(hiprt/lib)). Windows is to be handled by the CMakeLists.txt. Some recent change removed that code.
I was trying to place hiprt in the same location as the kernel binaries. I removed the installation of hiprt64.dll from the CMake file because it would place hiprt64.dll in the blender.shared. So, I must adjust the CMake to copy hiprt64 it to the Blender root directory instead. The way hiprt64 should be handled is quite confusing for me (and probably self inflicted). I want to make the code changes in the right place, but I also want to ensure that hiprt64.dll/so, hiprt02003_6.1_amd.hipfb, and oro_compiled_kernels.hipfb all end up in the same location.
If you follow what I've write then updating the standalone repository will be easy.
I thought I already did that. Can you clarify what part of the instruction you are referring to?
For example, the Updated the copy location of bvh kernel binaries includes changes to the way how the library is built (-
DBITCODE=ON), without mentioning this.
BITCODE has always been enabled. Earlier this week, when transferring my work from Linux to Windows, I mistakenly pushed changes into this branch. Please disregard any changes made in that commit. a73c214129
BITCODE allows HIP-RT to rely on precompiled binaries instead of compiling and caching them on first use. I briefly considered runtime compilation to eliminate the need for kernel binaries, but this would require users to have the HIP SDK installed.
some messages seem to suggest bitcode is not needed, but then that it actually is needed because of runtime,
but also there are some compilation errors mentioned related to Python and BITCODE. It is very confusing.
Python is not needed when BITCODE is enabled. HIP-RT was originally configured to load either precompiled kernels or encrypted kernel sources, with the latter requiring Python. But, these two components are now decoupled.
HIP-RT provides traversal and intersection functions, as well as BVH construction kernels. When HIP-RT was closed-source, the intersection functions were provided in the HIP-RT SDK as a precompiled library, which would be linked against the Cycles kernels at either build time or runtime. BVH kernels and the DLL were distributed through the AMD driver.
Now that the code is open-source, both the compilation and linking of intersection functions take place within Cycles. However, the process introduces handling of BVH kernels—hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb—which are executed when hiprtBuildGeometry and hiprtBuildScene are called:
Can you write a small design summary, making it more clear what are the moving parts, and when things are supposed to happen (AoT, JIT, etc)?
For end-users, these kernels must be compiled ahead of time. JIT compilation could be an option for developers but it requires the HIP-RT SDK to be installed, the HIPRT_PATH environment variable to be set, and the same HIP SDK version that compiled hiprt64 to be used. The last requirement makes the process too fragile and made me change the code to always compile hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb ahead of time.
It is not something that is 100% clean, and things seem to go back-n-forth in the code, which does not help understanding what's going on.
I hope the comments so far made things clearer. If not, let me know and I will add more context.
Once everything
It is a good idea to review own code after submission, to ensure PR contains everything you expect, and doesn't contain anything you don't expect.
Once you think everything is functionally correct and in line with Blender design, I will do a final check for cosmetic issues, dead or redundant code.
I don't really understand why those files could not provided explicitly via API. Similarly how render kernel does not need to follow any naming convention, and does not need to be in any
specific location w.r.t to driver or other libraries.
I agree with your assessment. HIP-RT was open-sourced without any changes to its original closed-source design, which was meant to keep the implementation of BVH construction and traversal methods secure and work around the absence of an intermediate format.
This project is owned by another team, and the turnaround time for code change/review and updating unit tests is slow. This delay could potentially extend beyond the deadline for merging this pull request in time for the 4.3 release. I appreciate that this isn’t your issue, and I understand that even temporarily placing the kernels (hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb) in the same location as hiprt64 conflicts with Blender’s design principles.
To address this, I created a fork of HIP-RT and added an option to bundle the kernel binaries into the hiprt library. I hope to eventually merge this change into the main repository.
I’d like to know your thoughts on using my HIP-RT fork to resolve this issue. The fork is linked to my AMD account, so in the event that I’m unavailable (e.g., run over by a bus), someone else could take over. This seems like a viable solution given the tight timeline. Alternatively, would you consider merging the pull request as it is and keeping the kernel binaries in their current, though suboptimal, location? If so, I’ll work with the HIP-RT team to ensure the bundling option is integrated into the main repository before the November release.
@Sergey
The explanations for the different parts are scattered across various threads, so I'll do my best to consolidate everything here.
>Please remove handling of libhiprt64.so and hiprt64.dll from this file.
Will do. I added it temporarily until the kernel binaries placement is sorted out (more on this at the end of the comments)
> For the purpose of this PR, Linux is handled via PLATFORM_BUNDLED_LIBRARIES (hiprt is added via add_bundled_library(hiprt/lib)). Windows is to be handled by the CMakeLists.txt. Some recent change removed that code.
I was trying to place hiprt in the same location as the kernel binaries. I removed the installation of hiprt64.dll from the CMake file because it would place hiprt64.dll in the blender.shared. So, I must adjust the CMake to copy hiprt64 it to the Blender root directory instead. The way hiprt64 should be handled is quite confusing for me (and probably self inflicted). I want to make the code changes in the right place, but I also want to ensure that hiprt64.dll/so, hiprt02003_6.1_amd.hipfb, and oro_compiled_kernels.hipfb all end up in the same location.
>If you follow what I've write then updating the standalone repository will be easy.
I thought I already did that. Can you clarify what part of the instruction you are referring to?
https://projects.blender.org/salipour/AMD_HIPRT/src/commit/7336078910ff48d4c3d85d2c0d5387770bddf97b/intern/cycles/kernel/CMakeLists.txt#L447
>For example, the Updated the copy location of bvh kernel binaries includes changes to the way how the library is built (-
DBITCODE=ON), without mentioning this.
BITCODE has always been enabled. Earlier this week, when transferring my work from Linux to Windows, I mistakenly pushed changes into this branch. Please disregard any changes made in that commit. https://projects.blender.org/blender/blender/commit/a73c214129e3aab7279e1e57120df6971a0f4a7d
BITCODE allows HIP-RT to rely on precompiled binaries instead of compiling and caching them on first use. I briefly considered runtime compilation to eliminate the need for kernel binaries, but this would require users to have the HIP SDK installed.
>some messages seem to suggest bitcode is not needed, but then that it actually is needed because of runtime,
but also there are some compilation errors mentioned related to Python and BITCODE. It is very confusing.
Python is not needed when BITCODE is enabled. HIP-RT was originally configured to load either precompiled kernels or encrypted kernel sources, with the latter requiring Python. But, these two components are now decoupled.
HIP-RT provides traversal and intersection functions, as well as BVH construction kernels. When HIP-RT was closed-source, the intersection functions were provided in the HIP-RT SDK as a precompiled library, which would be linked against the Cycles kernels at either build time or runtime. BVH kernels and the DLL were distributed through the AMD driver.
Now that the code is open-source, both the compilation and linking of intersection functions take place within Cycles. However, the process introduces handling of BVH kernels—hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb—which are executed when hiprtBuildGeometry and hiprtBuildScene are called:
https://projects.blender.org/salipour/AMD_HIPRT/src/commit/06076d74b1bec0883bfd63afd830a91acf254882/intern/cycles/device/hiprt/device_impl.cpp#L788 and https://projects.blender.org/salipour/AMD_HIPRT/src/commit/06076d74b1bec0883bfd63afd830a91acf254882/intern/cycles/device/hiprt/device_impl.cpp#L994
> Can you write a small design summary, making it more clear what are the moving parts, and when things are supposed to happen (AoT, JIT, etc)?
For end-users, these kernels must be compiled ahead of time. JIT compilation could be an option for developers but it requires the HIP-RT SDK to be installed, the HIPRT_PATH environment variable to be set, and the same HIP SDK version that compiled hiprt64 to be used. The last requirement makes the process too fragile and made me change the code to always compile hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb ahead of time.
https://projects.blender.org/salipour/AMD_HIPRT/src/commit/7336078910ff48d4c3d85d2c0d5387770bddf97b/intern/cycles/kernel/CMakeLists.txt#L729
> It is not something that is 100% clean, and things seem to go back-n-forth in the code, which does not help understanding what's going on.
I hope the comments so far made things clearer. If not, let me know and I will add more context.
Once everything
>It is a good idea to review own code after submission, to ensure PR contains everything you expect, and doesn't contain anything you don't expect.
Once you think everything is functionally correct and in line with Blender design, I will do a final check for cosmetic issues, dead or redundant code.
>I don't really understand why those files could not provided explicitly via API. Similarly how render kernel does not need to follow any naming convention, and does not need to be in any
specific location w.r.t to driver or other libraries.
I agree with your assessment. HIP-RT was open-sourced without any changes to its original closed-source design, which was meant to keep the implementation of BVH construction and traversal methods secure and work around the absence of an intermediate format.
This project is owned by another team, and the turnaround time for code change/review and updating unit tests is slow. This delay could potentially extend beyond the deadline for merging this pull request in time for the 4.3 release. I appreciate that this isn’t your issue, and I understand that even temporarily placing the kernels (hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb) in the same location as hiprt64 conflicts with Blender’s design principles.
To address this, I created a fork of HIP-RT and added an option to bundle the kernel binaries into the hiprt library. I hope to eventually merge this change into the main repository.
I’d like to know your thoughts on using my HIP-RT fork to resolve this issue. The fork is linked to my AMD account, so in the event that I’m unavailable (e.g., run over by a bus), someone else could take over. This seems like a viable solution given the tight timeline. Alternatively, would you consider merging the pull request as it is and keeping the kernel binaries in their current, though suboptimal, location? If so, I’ll work with the HIP-RT team to ensure the bundling option is integrated into the main repository before the November release.
@Sergey please let me know if my previous comments clear up confusion and also let me know how you would like to move forward with BVH binaries (do you want to use my HIP-RT fork?)
@Sergey please let me know if my previous comments clear up confusion and also let me know how you would like to move forward with BVH binaries (do you want to use my HIP-RT fork?)
Sahar A. Kashi
added 2 commits 2024-08-16 17:42:27 +02:00
@salipour Thanks for the explanation and updates. Feels like we're getting somewhere.
My plan is the following:
Double-check the libs build pipeline, and make a Blender build on Rocky 8 VM.
Adjust whatever feels needs adjustment.
Test it on a local machine to ensure any possible changes didn't break things.
Assuming I'll be using ubuntu 22.04 or 24.04 on the test machine, what is the driver version I'd need to have for tests?
@salipour Thanks for the explanation and updates. Feels like we're getting somewhere.
My plan is the following:
- Double-check the libs build pipeline, and make a Blender build on Rocky 8 VM.
- Adjust whatever feels needs adjustment.
- Test it on a local machine to ensure any possible changes didn't break things.
Assuming I'll be using ubuntu 22.04 or 24.04 on the test machine, what is the driver version I'd need to have for tests?
I tried testing this PR on Windows and it doesn't work. My testing points to it being related to hiprt64.dll not being installed anywhere that hiprtewInit() can find it.
Is this expected to be the case?
I tried testing this PR on Windows and it doesn't work. My testing points to it being related to `hiprt64.dll` not being installed anywhere that `hiprtewInit()` can find it.
Is this expected to be the case?
The hiprt64.dll needs to be installed by the CMakeLists.txt in the creator.
But, I am not 100% sure the LoadLibrary type of approach follows the manifest. Hopefully it does.
@Alaska Is a bit unclear whether you've run the `make deps` type of a thing. I assume you did :)
One of the obvious thing now when you mention it, we need to restore this part of the code:
https://projects.blender.org/blender/blender/commit/7358de3ee86b7122f5a8ad542dea2cdb819fe2e1#diff-e165d5b8de6cfd61c8a0c9b1d5b223f50c55c9f7
The `hiprt64.dll` needs to be installed by the CMakeLists.txt in the creator.
But, I am not 100% sure the LoadLibrary type of approach follows the manifest. Hopefully it does.
@Alaska Is a bit unclear whether you've run the make deps type of a thing. I assume you did :)
Sorry I should of explain that better.
I ran make deps, picked out the HIP-RT library from that and put it in my libs folder. Then created a new build of Blender.
The issue I observed was that HIP-RT was greyed out in preferences. So I tracked down the code to hiprtewInit(), which was returning false (telling Cycles that the GPU didn't support HIPRT) because it couldn't find hiprt64.dll
I just manually copied hiprt64.dll to beside the Blender executable and that fixed the issue and allowed me to move on to testing. And I can confirm it is actually HIPRT as it shows some HIPRT specific artifacts I've seen in the past, and the artifacts disappear switching back to BVH2.
Another thing I noticed while building was that make with a configuration below release failed. E.g. make nobuild
CMake Error at intern/cycles/kernel/CMakeLists.txt:1399 (delayed_install):
delayed_install Function invoked with incorrect arguments for function
named: delayed_install
CMake Error at intern/cycles/kernel/CMakeLists.txt:1400 (delayed_install):
delayed_install Function invoked with incorrect arguments for function
named: delayed_install
The error seems to occur because HIPRT_INCLUDE_DIR isn't defined yet (because WITH_CYCLES_DEVICE_HIPRT isn't enabled in this configuration).
> @Alaska Is a bit unclear whether you've run the `make deps` type of a thing. I assume you did :)
<details>
<summary>Sorry I should of explain that better.</summary>
I ran `make deps`, picked out the HIP-RT library from that and put it in my libs folder. Then created a new build of Blender.
The issue I observed was that HIP-RT was greyed out in preferences. So I tracked down the code to `hiprtewInit()`, which was returning false (telling Cycles that the GPU didn't support HIPRT) because it couldn't find `hiprt64.dll`
I just manually copied `hiprt64.dll` to beside the Blender executable and that fixed the issue and allowed me to move on to testing. And I can confirm it is actually HIPRT as it shows some HIPRT specific artifacts I've seen in the past, and the artifacts disappear switching back to BVH2.
</details>
---
Another thing I noticed while building was that `make` with a configuration below `release` failed. E.g. `make nobuild`
```
CMake Error at intern/cycles/kernel/CMakeLists.txt:1399 (delayed_install):
delayed_install Function invoked with incorrect arguments for function
named: delayed_install
CMake Error at intern/cycles/kernel/CMakeLists.txt:1400 (delayed_install):
delayed_install Function invoked with incorrect arguments for function
named: delayed_install
```
The error seems to occur because `HIPRT_INCLUDE_DIR` isn't defined yet (because `WITH_CYCLES_DEVICE_HIPRT` isn't enabled in this configuration).
A quick recap from testing on Windows with a RX 7800XT.
Running automated render tests and benchmarking, there are no performance, memory, or render regressions that I can find. And in fact performance is improved in some scenes and some issues have been fixed.
I have not tested viewport stability/reliability, and Linux obviously still needs testing.
A quick recap from testing on Windows with a RX 7800XT.
Running automated render tests and benchmarking, there are no performance, memory, or render regressions that I can find. And in fact performance is improved in some scenes and some issues have been fixed.
I have not tested viewport stability/reliability, and Linux obviously still needs testing.
The error seems to occur because HIPRT_INCLUDE_DIR isn't defined yet (because WITH_CYCLES_DEVICE_HIPRT isn't enabled in this configuration).
Ah! The check for WITH_HIPRT is left behind on my local system.
I tried testing this PR on Windows and it doesn't work. My testing points to it being related to hiprt64.dll not being installed
@Alaska did you copy hiprt64.dll manually? How did you make it work? I have to update the code to copy the dll to the root directory.
> The error seems to occur because `HIPRT_INCLUDE_DIR` isn't defined yet (because `WITH_CYCLES_DEVICE_HIPRT` isn't enabled in this configuration).
Ah! The check for WITH_HIPRT is left behind on my local system.
>I tried testing this PR on Windows and it doesn't work. My testing points to it being related to hiprt64.dll not being installed
@Alaska did you copy hiprt64.dll manually? How did you make it work? I have to update the code to copy the dll to the root directory.
>Assuming I'll be using ubuntu 22.04 or 24.04 on the test machine, what is the driver version I'd need to have for tests?
@sergey any recent driver should work.
Sahar A. Kashi
added 1 commit 2024-08-21 00:40:17 +02:00
@Alaska did you copy hiprt64.dll manually? How did you make it work?
When it wasn't working I hadn't copied it manually. But later on I copied it manually and used that for my regression and performance testing.
> @Alaska did you copy hiprt64.dll manually? How did you make it work?
When it wasn't working I hadn't copied it manually. But later on I copied it manually and used that for my regression and performance testing.
Sahar A. Kashi
added 1 commit 2024-08-21 20:58:41 +02:00
@Sergey for e4ae1b5533 I did
build the dependencies on Rocky 8 and Ubuntu.
build Blender with HIP-RT enabled on Ubuntu and tested rendering.
build Blender with HIP-RT enabled on Windows as well and briefly tested rendering.
Our QA will do extensive testing once the daily builds become available for this PR.
Let me know if you have any concerns.
@Sergey for https://projects.blender.org/blender/blender/commit/e4ae1b5533720ced095311845c13eaf5a0a309e6 I did
build the dependencies on Rocky 8 and Ubuntu.
build Blender with HIP-RT enabled on Ubuntu and tested rendering.
build Blender with HIP-RT enabled on Windows as well and briefly tested rendering.
Our QA will do extensive testing once the daily builds become available for this PR.
Let me know if you have any concerns.
I've been testing the patch for building libraries on Rocky 8, and Blender on Ubuntu 22.04. It seems to work with a bit of caveats I'll come back to in a moment. The render time on Junkshop dropped from 22 seconds to 16 on WX7800. That's nice :)
The issues I've run into:
There seems to be a memory leak, unrelated to this change. I've submitted #126788. I'm not a huge fan of it, but I wanted to make something safe and easy for possible porting to the 4.2 LTS. Maybe you can verify it?
The kernel_rt_gfx.hipfb takes about 30min to compile with rocm 6.1.2 and 6.1.3 (the latter one is what came with the driver on Ubuntu). That's a bit unideal, as it risks stalling our CI/CD pipeline. There was some discussion earlier on, and 6.1.1 seemed to work. Can you confirm the compilation times on your end?
At this point I wouldn't mind getting extra eyes from the platform maintainers (@LazyDodo and @ideasman42) to do the review.
@salipour Thanks for the updates.
I've been testing the patch for building libraries on Rocky 8, and Blender on Ubuntu 22.04. It seems to work with a bit of caveats I'll come back to in a moment. The render time on Junkshop dropped from 22 seconds to 16 on WX7800. That's nice :)
The issues I've run into:
- There seems to be a memory leak, unrelated to this change. I've submitted #126788. I'm not a huge fan of it, but I wanted to make something safe and easy for possible porting to the 4.2 LTS. Maybe you can verify it?
- The `kernel_rt_gfx.hipfb` takes about 30min to compile with rocm 6.1.2 and 6.1.3 (the latter one is what came with the driver on Ubuntu). That's a bit unideal, as it risks stalling our CI/CD pipeline. There was some discussion earlier on, and 6.1.1 seemed to work. Can you confirm the compilation times on your end?
At this point I wouldn't mind getting extra eyes from the platform maintainers (@LazyDodo and @ideasman42) to do the review.
The kernel_rt_gfx.hipfb takes about 30min to compile with rocm 6.1.2 and 6.1.3
More or less the same for me (although I have a very old cpu on my Ubuntu machine.) We can mitigate it by some changes on the HIP-RT library to have compile and link in one step but for the moment, I will break down the compilation into individual binaries per architecture in the CMake file. It will probably add up to the same amount of time, but no single compilation task will take more than couple of minutes then.
> The kernel_rt_gfx.hipfb takes about 30min to compile with rocm 6.1.2 and 6.1.3
More or less the same for me (although I have a very old cpu on my Ubuntu machine.) We can mitigate it by some changes on the HIP-RT library to have compile and link in one step but for the moment, I will break down the compilation into individual binaries per architecture in the CMake file. It will probably add up to the same amount of time, but no single compilation task will take more than couple of minutes then.
For me the timing is about 16min on i9-11900K, but it goes up to 30min when running in a VM. This only includes compilation time kernel_rt_gfx.hiprt. The configuration of similar CPU + VM depicts the hardware used on the buildbot quite well. So it is a bit worrying to see such step added. The time is for kernel_rt_gfx.bc andkernel_rt_gfx.hiprt btw.
I will break down the compilation into individual binaries per architecture in the CMake file
Do you mean we can break down the kernel_rt_gfx into individual architectures?
Is there a flag we can pass to the HIP compiler to build more things in parallel at a cost of extra memory use (similar to -fsycl-max-parallel-link-jobs) ?
Having more smaller tasks running in parallel should help preventing situation when one big task is compiled at the end of a build process and clogs the entire pipeline (we don't exactly have a way to control order of kernels to be compiled).
For me the timing is about 16min on i9-11900K, but it goes up to 30min when running in a VM. This only includes compilation time `kernel_rt_gfx.hiprt`. The configuration of similar CPU + VM depicts the hardware used on the buildbot quite well. So it is a bit worrying to see such step added. The time is for `kernel_rt_gfx.bc` and`kernel_rt_gfx.hiprt` btw.
> I will break down the compilation into individual binaries per architecture in the CMake file
Do you mean we can break down the `kernel_rt_gfx` into individual architectures?
Is there a flag we can pass to the HIP compiler to build more things in parallel at a cost of extra memory use (similar to `-fsycl-max-parallel-link-jobs`) ?
Having more smaller tasks running in parallel should help preventing situation when one big task is compiled at the end of a build process and clogs the entire pipeline (we don't exactly have a way to control order of kernels to be compiled).
Do you mean we can break down the kernel_rt_gfx into individual architectures?
Yes.
Is there a flag we can pass to the HIP compiler to build more things in parallel?
Yes, -parallel-jobs should do it.
Update:: @Sergey the change will look like something like aa54a52051
I compiled and run Blender on Ubuntu, and while it works without a problem, something is not right. The hipfb files keep growing in size as if later binaries in the list are built for multiple targets. I have to go over the changes in the cmake more carefully .
> Do you mean we can break down the kernel_rt_gfx into individual architectures?
Yes.
> Is there a flag we can pass to the HIP compiler to build more things in parallel?
Yes, -parallel-jobs should do it.
Update::
@Sergey the change will look like something like https://projects.blender.org/salipour/AMD_HIPRT/commit/aa54a52051aea220727f75491cf595295a1d0bd7
I compiled and run Blender on Ubuntu, and while it works without a problem, something is not right. The hipfb files keep growing in size as if later binaries in the list are built for multiple targets. I have to go over the changes in the cmake more carefully .
@salipour I had a quick look into the -parallel-jobs. The results are quire cool, actually. With 4 parallel jobs the compilation time toughly 1/4, and (unless i've missed something) the memory usage stays within reasonable amounts.
I've attached the patch for reference.
Not sure it worth going more complicated route with splitting tasks on CMake level if we can use a simpler solution.
I think it would be fine to add an option similar to the SYCL_OFFLINE_COMPILER_PARALLEL_JOBS:
set(HIPRT_COMPILER_PARALLEL_JOBS 1 CACHE STRING "Number of parallel compiler instances to use for HIP-RT targets")
mark_as_advanced(HIPRT_COMPILER_PARALLEL_JOBS)
And then whenever applicable, to -parallel-jobs=${HIPRT_COMPILER_PARALLEL_JOBS}
For the buildbot we'll set the value from the build pipeline configuration, and keep compile time manageable.
@salipour I had a quick look into the `-parallel-jobs`. The results are quire cool, actually. With 4 parallel jobs the compilation time toughly 1/4, and (unless i've missed something) the memory usage stays within reasonable amounts.
I've attached the patch for reference.
Not sure it worth going more complicated route with splitting tasks on CMake level if we can use a simpler solution.
I think it would be fine to add an option similar to the `SYCL_OFFLINE_COMPILER_PARALLEL_JOBS`:
```
set(HIPRT_COMPILER_PARALLEL_JOBS 1 CACHE STRING "Number of parallel compiler instances to use for HIP-RT targets")
mark_as_advanced(HIPRT_COMPILER_PARALLEL_JOBS)
```
And then whenever applicable, to `-parallel-jobs=${HIPRT_COMPILER_PARALLEL_JOBS}`
For the buildbot we'll set the value from the build pipeline configuration, and keep compile time manageable.
Good! If this option reliably works, then I'd much rather stick with the way it is set up now, rather than splitting the work like I did in the change list I referenced earlier.
> Not sure it worth going more complicated route
Good! If this option reliably works, then I'd much rather stick with the way it is set up now, rather than splitting the work like I did in the change list I referenced earlier.
The default value is 1 which has no effect on the compilation time
compared to the state prior to this change.
With the value of 4 the setup which is similar to the buildbot the
compile time for the new kernels is lowered from 30+ to about min,
which is more acceptable. There does not seep to be huge memory
penalty, so the value might potentially be increased further if
needed.
@salipour I've committed the -parallel-jobs to the PR, as well as some tweaks to the CI/CD configuration.
I don't think I have stoppers from my side now.
There are a few things I'd like to simplify in Cycles w.r.t the way we handle kernels and such, but it should happen for all platforms, and after this PR has landed.
We still need to ensure platform maintainers reviewed the change, and possibly sync-up for the library updates.
@salipour I've committed the `-parallel-jobs` to the PR, as well as some tweaks to the CI/CD configuration.
I don't think I have stoppers from my side now.
There are a few things I'd like to simplify in Cycles w.r.t the way we handle kernels and such, but it should happen for all platforms, and after this PR has landed.
We still need to ensure platform maintainers reviewed the change, and possibly sync-up for the library updates.
Thank you @Sergey
I'm glad the PR is now in a reasonable shape. @LazyDodo and @ideasman42 let me know what you think.
I will be away until next Tuesday. If there are any concern, I will address it after I return.
Thank you @Sergey
I'm glad the PR is now in a reasonable shape.
@LazyDodo and @ideasman42 let me know what you think.
I will be away until next Tuesday. If there are any concern, I will address it after I return.
this causes a whole bunch of .cpp files inside the final libdir, the root cause is when running the install phase of the hiprt project it does not install any of its own headers and build_files\build_environment\cmake\hiprt.cmake ends up pillaging the source folder to get them. this is not how that is supposed to work. This needs to be fixed in upstream hiprt.
this causes a whole bunch of .cpp files inside the final libdir, the root cause is when running the install phase of the hiprt project it does not install any of its own headers and `build_files\build_environment\cmake\hiprt.cmake` ends up pillaging the source folder to get them. this is not how that is supposed to work. This needs to be fixed in upstream hiprt.
I will add this (hiprt not installing any of its headers) to the list of changes I am compiling to hand over to the hiprt team. The project does install some of its headers (in the dist folder) but only the ones required in a closed-source setup.
I will add this (hiprt not installing any of its headers) to the list of changes I am compiling to hand over to the hiprt team. The project does install some of its headers (in the dist folder) but only the ones required in a closed-source setup.
@LazyDodo
HIP-RT now handles installation of its own headers and the change is public. I updated the CMake files to pick up the latest version of HIP-RT and removed unnecessary copies db7dc075bc
Let me know if everything looks okay now.
@LazyDodo
HIP-RT now handles installation of its own headers and the change is public. I updated the CMake files to pick up the latest version of HIP-RT and removed unnecessary copies db7dc075bc1851eacebf845b9d71d952af5874f7
Let me know if everything looks okay now.
HIP_LINKER_EXECUTABLE is a hip component, checking on it here leads to the following bizarre log when it can't be found:
-- Could NOT find HIP (missing: HIP_HIPCC_EXECUTABLE) (Required is at least version "5.5.0")
-- Found HIPRT: K:/BlenderGit/blender/lib/windows_x64/hiprt/include
-- Could NOT find HIPRT (missing: HIP_LINKER_EXECUTABLE)
-- HIP RT not found, disabling WITH_CYCLES_DEVICE_HIPRT
Found HIPRT! .... excellent! .......Could NOT find HIPRT ....wait..that's not what you just told me... you told me you found it.. (missing: HIP_LINKER_EXECUTABLE)... hold up that's not even a hiprt component! that's a hip component!
This check should be in FindHIP.cmake which is the module that actually locates HIP_LINKER_EXECUTABLE
`HIP_LINKER_EXECUTABLE` is a hip component, checking on it here leads to the following bizarre log when it can't be found:
```
-- Could NOT find HIP (missing: HIP_HIPCC_EXECUTABLE) (Required is at least version "5.5.0")
-- Found HIPRT: K:/BlenderGit/blender/lib/windows_x64/hiprt/include
-- Could NOT find HIPRT (missing: HIP_LINKER_EXECUTABLE)
-- HIP RT not found, disabling WITH_CYCLES_DEVICE_HIPRT
```
`Found HIPRT!` .... excellent! .......`Could NOT find HIPRT` ....wait..that's not what you just told me... you told me you found it.. `(missing: HIP_LINKER_EXECUTABLE)`... hold up that's not even a hiprt component! that's a hip component!
This check should be in `FindHIP.cmake` which is the module that actually locates `HIP_LINKER_EXECUTABLE`
HIP_LINKER_EXECUTABLE which points to clang++ is a HIP component but if it is missing, it only affects enabling HIP-RT. When HIP-RT headers are present, Found HIPRT is appropriate, but if the HIP component required to compile the HIP-RT kernels is missing, then HIP-RT must be disabled. So, a more appropriate message instead of "Could NOT find HIPRT (missing: HIP_LINKER_EXECUTABLE)" would be something like "Could NOT find HIPRT dependency (missing: HIP_LINKER_EXECUTABLE)." Alternatively, if HIP is not found, then there is no need to look for HIP-RT.
HIP_LINKER_EXECUTABLE which points to clang++ is a HIP component but if it is missing, it only affects enabling HIP-RT. When HIP-RT headers are present, Found HIPRT is appropriate, but if the HIP component required to compile the HIP-RT kernels is missing, then HIP-RT must be disabled. So, a more appropriate message instead of "Could NOT find HIPRT (missing: HIP_LINKER_EXECUTABLE)" would be something like "Could NOT find HIPRT dependency (missing: HIP_LINKER_EXECUTABLE)." Alternatively, if HIP is not found, then there is no need to look for HIP-RT.
if HIP is not found, then there is no need to look for HIP-RT.
That's a valid point, ideally the code around here could be changed to something like the following:
if(WITH_CYCLES_DEVICE_HIP)if(WITH_CYCLES_HIP_BINARIES)set(_HIP_COMPONENTS"hipcc_executable")if(WITH_CYCLES_DEVICE_HIPRT)list(APPEND_HIP_COMPONENTS"hiplinker_executable")endif()# Need at least HIP 5.5 to solve compiler bug affecting the kernel.
find_package(HIP5.5.0REQUIREDCOMPONENTS${_HIP_COMPONENTS})unset(__HIP_COMPONENTS)set_and_warn_library_found("HIP compiler"HIP_FOUNDWITH_CYCLES_HIP_BINARIES)if(HIP_FOUND)message(STATUS"Found HIP ${HIP_HIPCC_EXECUTABLE} (${HIP_VERSION})")if(WITH_CYCLES_DEVICE_HIPRT)find_package(HIPRT)set_and_warn_library_found("HIP RT"HIPRT_FOUNDWITH_CYCLES_DEVICE_HIPRT)endif()endif()endif()endif()
This naturally does mean, findhip.cmake needs to be updated to deal with the optional components or we just accept the linker always will have to be found in which case we can skip the components part and just add a check it's set in findhip.cmake
I have a slight lean towards just always requiring the linker since its the least amount of work, unsure how @Sergey feels about this.
> if HIP is not found, then there is no need to look for HIP-RT.
That's a valid point, ideally the code around [here](https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/cmake/external_libs.cmake#L45) could be changed to something like the following:
```CMake
if(WITH_CYCLES_DEVICE_HIP)
if(WITH_CYCLES_HIP_BINARIES)
set(_HIP_COMPONENTS "hipcc_executable")
if(WITH_CYCLES_DEVICE_HIPRT)
list(APPEND _HIP_COMPONENTS "hiplinker_executable")
endif()
# Need at least HIP 5.5 to solve compiler bug affecting the kernel.
find_package(HIP 5.5.0 REQUIRED COMPONENTS ${_HIP_COMPONENTS})
unset(__HIP_COMPONENTS)
set_and_warn_library_found("HIP compiler" HIP_FOUND WITH_CYCLES_HIP_BINARIES)
if(HIP_FOUND)
message(STATUS "Found HIP ${HIP_HIPCC_EXECUTABLE} (${HIP_VERSION})")
if(WITH_CYCLES_DEVICE_HIPRT)
find_package(HIPRT)
set_and_warn_library_found("HIP RT" HIPRT_FOUND WITH_CYCLES_DEVICE_HIPRT)
endif()
endif()
endif()
endif()
```
This naturally does mean, `findhip.cmake` needs to be updated to deal with the optional components or we just accept the linker always will have to be found in which case we can skip the components part and just add a check it's set in `findhip.cmake`
I have a slight lean towards just always requiring the linker since its the least amount of work, unsure how @Sergey feels about this.
I have a slight lean towards just always requiring the linker since
Originally, HIPCC_EXECUTABLE was required but not HIP_LINKER_EXECUTABLE. At some point, I noticed the linker was no longer found #121050 (comment) least the way my system was setup) and it led to kernel compilation failure during the build. So, I made the change to make the linker required for HIP-RT. If we can make sure that if HIPCC_EXECUTABLE is found then HIP_LINKER_EXECUTABLE is found, then always requiring the linker should be fine.
> I have a slight lean towards just always requiring the linker since
Originally, HIPCC_EXECUTABLE was required but not HIP_LINKER_EXECUTABLE. At some point, I noticed the linker was no longer found https://projects.blender.org/blender/blender/pulls/121050#issuecomment-1243443(at least the way my system was setup) and it led to kernel compilation failure during the build. So, I made the change to make the linker required for HIP-RT. If we can make sure that if HIPCC_EXECUTABLE is found then HIP_LINKER_EXECUTABLE is found, then always requiring the linker should be fine.
@LazyDodo and @Sergey
That's the simplest way I could come up with 9333515ce3
Let me know if I need to make changes.
@LazyDodo and @Sergey
That's the simplest way I could come up with https://projects.blender.org/blender/blender/commit/9333515ce3feca8054074ea758788a31e86f1860
Let me know if I need to make changes.
@LazyDodo@salipour Are there remaining points that needs addressing on Windows?
The .cpp files in the include folder still bother me, but given i also really want this diff to be finally off my plate, i'll take a commitment the hiprt team will fix this in a future update.
> @LazyDodo @salipour Are there remaining points that needs addressing on Windows?
The .cpp files in the include folder still bother me, but given i also really want this diff to be finally off my plate, i'll take a commitment the hiprt team will fix this in a future update.
The .cpp files in the include folder still bother me, but given i also really want this diff to be finally off my plate, i'll take a commitment the hiprt team will fix this in a future update.
@LazyDodo the change to address this issue on hiprt side is done and pending review internally. It will eventually make it to the public repo.
> The .cpp files in the include folder still bother me, but given i also really want this diff to be finally off my plate, i'll take a commitment the hiprt team will fix this in a future update.
@LazyDodo the change to address this issue on hiprt side is done and pending review internally. It will eventually make it to the public repo.
Just to be sure, @Alaska you tested this on actual hardware on windows, correct?
Two weeks ago, I tested it on a RX 7800XT on Windows, but had to make some manual modifications to get it to work. Once it was working things seemed to be fine (no obvious regressions). Based on the commits I believe all the issues that required manual modification have been resolved.
I can retest it again with all the latest changes if you'd like.
> Just to be sure, @Alaska you tested this on actual hardware on windows, correct?
Two weeks ago, I tested it on a RX 7800XT on Windows, but had to make some manual modifications to get it to work. Once it was working things seemed to be fine (no obvious regressions). Based on the commits I believe all the issues that required manual modification have been resolved.
I can retest it again with all the latest changes if you'd like.
All the build issues I encountered 2 weeks ago appear to be resolved and there were no new issues I could identify. Now it's as simple to setup and build as current main (ignoring the requirement to build the HIP-RT library).
Running the same tests I did previously (mainly the Cycles render tests), there's no obvious regressions, and the upgrading of the HIP-RT library as part of this PR improves some areas.
All the build issues I encountered 2 weeks ago appear to be resolved and there were no new issues I could identify. Now it's as simple to setup and build as current main (ignoring the requirement to build the HIP-RT library).
Running the same tests I did previously (mainly the [Cycles render tests](https://projects.blender.org/blender/blender-test-data/src/branch/main/render)), there's no obvious regressions, and the upgrading of the HIP-RT library as part of this PR improves some areas.
Sahar A. Kashi
added 2 commits 2024-09-10 03:18:15 +02:00
Rebuilding libs, will test this PR when they're done.
Update: builds & tests pass (accepting).
> @ideasman42 Did you have a chance to have a look?
Rebuilding libs, will test this PR when they're done.
-----
Update: builds & tests pass (accepting).
@LazyDodo@ideasman42 Do you think it's possible to have libraries landed before this patch?
I don't think it will compile with the current state of libraries and wrangler. And if we can have libraries available in git-lfs already we can easily land this patch, without going into complications of either temporarily disabling HIP-RT, or trying to support both old/new style of libraries.
I've tweaked the description to explain a bit in details what it is about. Maybe @salipour would want to have another pass on it.
I was trying to get benchmark numbers, but it is a bit hard until #127464 is resolved (at least to have representative numbers). Don't think that this bug is a stopper for the this PR though.
@LazyDodo @ideasman42 Do you think it's possible to have libraries landed before this patch?
I don't think it will compile with the current state of libraries and wrangler. And if we can have libraries available in git-lfs already we can easily land this patch, without going into complications of either temporarily disabling HIP-RT, or trying to support both old/new style of libraries.
I've tweaked the description to explain a bit in details what it is about. Maybe @salipour would want to have another pass on it.
I was trying to get benchmark numbers, but it is a bit hard until #127464 is resolved (at least to have representative numbers). Don't think that this bug is a stopper for the this PR though.
we could just land the builder changes, wait for the platform devs to do their thing, then land/test the rest of this PR once its in place
the platform devs could build the libs off this PR, upload the libs, and the builder changes will come retroactively with the rest of the PR
Personally i prefer 1
two ways we could go about that
1) we could just land the builder changes, wait for the platform devs to do their thing, then land/test the rest of this PR once its in place
2) the platform devs could build the libs off this PR, upload the libs, and the builder changes will come retroactively with the rest of the PR
Personally i prefer 1
@LazyDodo 1/2 both seem OK.
To avoid delay from being in different time-zones, I'll commit the libs to a branch.
It might even be best to do this preemptively once the libs have been built & tested as it can avoid doing this all over again.
@LazyDodo 1/2 both seem OK.
To avoid delay from being in different time-zones, I'll commit the libs to a branch.
It might even be best to do this preemptively once the libs have been built & tested as it can avoid doing this all over again.
@salipour Do you mind moving the change done to the dependency builder (build_environment folder) to a separate PR, so we can land that part with the libraries?
@salipour Do you mind moving the change done to the dependency builder (`build_environment` folder) to a separate PR, so we can land that part with the libraries?
@salipour That is exactly what I had in mind.
For the next steps lets wait for the libraries to become available. I can help with making this PR pointing to a proper submodule then. Meanwhile maybe you can merge the main into this PR? Hopefully Git will be smart enough to recognize that build system part of it is the same, and will do the proper thing.
@salipour That is exactly what I had in mind.
For the next steps lets wait for the libraries to become available. I can help with making this PR pointing to a proper submodule then. Meanwhile maybe you can merge the `main` into this PR? Hopefully Git will be smart enough to recognize that build system part of it is the same, and will do the proper thing.
This change switches Cycles to an opensource HIP-RT library which
implements hardware ray-tracing. This library is now used on
both Windows and Linux. While there should be no noticeable changes
on Windows, on Linux this adds support for hardware ray-tracing on
AMD GPUs.
The majority of the change is typical platform code to add new
library to the dependency builder, and a change in the way how
ahead-of-time (AoT) kernels are compiled. There are changes in
Cycles itself, but they are rather straightforward: some APIs
changed in the opensource version of the library.
There are a couple of extra files which are needed for this to
work: hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb.
There are some assumptions in the HIP-RT library about how they
are available. Currently they follow the same rule as AoT
kernels for oneAPI:
Performance comparison on Ubuntu 22.04.5:
info_cfg_option(WITH_CYCLES_DEVICE_HIPRT)insideCmakeLists.txtshould be inside theif (NOT APPLE)check above it.Generally we try to make things work with old and new versions of a library, to make coordination of landing updates easier. But in this case it looks not so simple, and not worth doing.
So I think when the time comes to land this, we either disable
WITH_CYCLES_DEVICE_HIPRTfor a little bit, or coordinate with @LazyDodo to land precompiled libraries at the same time.I wasn't able to actually test the
make depsintegration because the version doesn't seem to exist. But some comments from reading through the changes.@ -699,3 +697,1 @@option(WITH_CYCLES_DEVICE_HIPRT "Enable Cycles AMD HIPRT support" OFF)mark_as_advanced(WITH_CYCLES_DEVICE_HIPRT)endif()option(WITH_CYCLES_DEVICE_HIPRT "Enable Cycles AMD HIPRT support" ON)This should still default to OFF.
It's
build_files/cmake/config/blender_release.cmakethat enables it by default for releases. Though we should still not enable it for Linux there, until after the precompiled libraries for that platform have landed.@ -0,0 +8,4 @@set(HIPRT_CMAKE_FLAGS ${DEFAULT_CMAKE_FLAGS})set(HIPRT_EXTRA_ARGS--DHIPRT_EXPORTS=ONShould be
-Dinstead of--D?@ -0,0 +39,4 @@)if(WIN32)if(BUILD_MODE STREQUAL Release)For Linux, there will need to be additions in
build_files/build_environment/cmake/harvest.cmaketo install things.I guess something like this, not sure if we want to just install all files or only a subset.
@ -0,0 +54,4 @@ExternalProject_Add_Step(external_hiprt after_installCOMMAND ${CMAKE_COMMAND} -E copy${LIBDIR}/hiprt/dist/bin/Release/hiprt64.dll${HARVEST_TARGET}/hiprt/bin/hiprt64.dllI think
hiprt64_d.dllshould be used here.@ -856,2 +856,4 @@set(PYBIND11_HASH_TYPE MD5)set(PYBIND11_FILE pybind-v${PYBIND11_VERSION}.tar.gz)set(HIPRT_VERSION 2.3.7df94af)This version doesn't seem to exist?
https://github.com/GPUOpen-LibrariesAndSDKs/HIPRTSDK/releases
Not sure if we need to wait for it to be released, or if another version should be used.
URI is wrong, it's over at https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/releases/tag/2.3.7df94af
Ah ok, so it's:
probably? I haven't gotten around to building this yet i'll try to schedule some time for this over the weekend.
Ok, thanks. For reference, this PR is heavily WIP and will need many more changes to get it to actually build.
I asked to just add the code so we get an idea of how things should fit together, what the build options should be, etc. But not to actually test if it works, since it's not so easy to build the libs on Windows.
@salipour, this URL was not fixed yet.
@ -51,0 +44,4 @@if(WIN32)set(HIPRT_DYNAMIC_LIB hiprt${HIPRT_VERSION}64.dll)else()set(HIPRT_DYNAMIC_LIB hiprt${HIPRT_VERSION}64.so)Is
${HIPRT_VERSION}supposed to be in the name? It's not inhiprt.cmake.I updated the name in hiprt.cmake file.
@ -53,0 +57,4 @@)if(HIPRT_LIB_DIR)set(HIPRT_DYNAMIC_LIB_PATHThis seems to be unused currently. I guess the idea is still to dynamically load the hiprt library rather than Blender linking to it? If so I guess this variable can be removed.
To install the shared libraries along with Blender, this will need to be added in
build_files/cmake/platform/platform_unix.cmake, which will copy all.sofiles into the bundled lib directory.For Windows this is done in
source/creator/CMakeLists.txt, something like:Unless there are separate release and debug libraries, in which case:
The main intention was to check if the dynamic library for hiprt exists before enabling it.
@ -732,0 +797,4 @@COMMAND ${hiprt_compile_command} ${hiprt_compile_flags_sdk_bc}DEPENDS ${HIPRT_INCLUDE_DIR}/hiprt/impl/hiprt_kernels_bitcode.h)Nitpick: use 1 instead of 2 empty lines.
@ -1 +1 @@Subproject commit a5521c85e03bfd1556ff1e63bf7163235c401497Subproject commit 19b2b87f5ef0d8caa39e0882fbf832052974b785This change to the submodule hash should be left out.
Happened accidentally. How can I roll it back?
Like this:
The submodule change is to be rolled back again.
This change came back again.
@brecht I made changes according to the feedback. What's the next step to move forward with this pull request?
Sorry for asking here, but expected to be in blender 4.2 release? or 4.3+
4.2
This is not going to be in Blender 4.2. Even if we get this landed soon, there is not enough time in the release cycle for this to get enough user testing and to fix potential issues. Bigger changes like this should land earlier.
I've been working on getting this to build on Linux, but ran into many issues. I pushed what I have so far but, there are still problems to resolve.
I ran into a similar issue when i was scripting manifold for the modelling module, ( #120712 ) I solved it there by downloading and unpacking the dep normally but setting the build command to echo "." and copying in the code in the patch step of the project that depends on it, not ideal, but likely the best we can do.
The changes you did seem like they make sense, but on windows this will require some more work since hiprt is using C++20 features (source_location) which isn't gonna be available on c++17 for msvc.
I gotta admit i'm a bit confused this got put up for review given the state it is in.
Let me check with HIPRT team and see why they haven't provided a CMake version yet. At any rate since you are calling Premake and passing bitcode option in hiprt.cmake the rest of the options are already taken care of.
HIPRT_EXPORTS and USE_HIP are already defined in Premake file by default.
bitcode --> HIPRT_BITCODE_LINKING and ORO_PRECOMPILED (Generating binaries for BVH kernel with the offline compiler)
bakeKernel --> HIPRT_LOAD_FROM_STRING (no longer necessary, mutually exclusive with above)
Okay! Let me also get back to you on this and see if the team can do it in a cleaner way.
I don't think easy encryption is needed anymore; the code is open source and even then, kernel compilation is offline.
I experienced something similar in the past with HIPRT 2.2 but not anymore with the latest compiler. What is your compiler version?
Well, that was the advice, to quickly put something together and make a WIP pull request. Anyway, let me know whatever I can do to facilitate this process. Unfortunately, I stopped receiving notification whatsoever from projects.blender but will check the page regularly to not miss any communication.
I made further changes to properly handle Orochi, disable encryption, some fixes for Windows (but still untested), fix library detection on Linux.
Next steps would be:
build_files/build_environment/patches/hiprt.diff.std::source_locationis C++20.Further, a CMake based build would help though I guess now that we have it working it's not so important. Still if Linux distributions want to package hiprt for Blender, they probably would want a better solution than using a bundled premake binary, some solution to get the submodule, and an install target which properly install the files.
For the long build time, I was using an older compiler. With hipcc 6.1.1 it seems reasonable. Probably we should upgrade ROCm on the buildbots along with this.
I forgot the C++20 issue. @salipour we could also use help to make HIPRT build with C++17. That's the version used by the VFX reference platform (https://vfxplatform.com/) that Blender and other software follows.
i'd hold off on upstreaming these patches, there will be some work required for windows.
first off, the low hanging fruit
nothing too crazy there, next problems i haven't solved yet, i may get to it over the weekend, but i'll have to dive in to their build system a little bit.
Open problems:
python somescript.pyduring the build , i'll have to see if i can fudge the path to include our python version.I won't have time today to fix these, but i'll try to make some time over the weekend.
I guess we can make the build system support providing a
PYTHON_PATH. To implement that in the cleanest way possible I guess it would be best to replace the platform specific bat and bash files with a single Python script. I was a bit hesistant to start refactoring too much in HIPRT, but I guess it's not that difficult if we want to do that and upstream is open to it.Fixed most issues, and it's in a build-able state now, but some trivial things still remain i'll have to address
hiprt0200364.libtohiprt.libbut the final binary that links it will always look forhiprt0200364.dllunsure what we want to do here. we can either go with the versioned names, or try to root out the version number wherever it is set.We don't link against the stub library. HIP-RT dill is loaded dynamically. I've been doing this on Windows without problem
Sure, will do. Also, one of my colleagues is working to add CMake support and include the submodules in the release package.
Ah yes, technically we don't even need the .lib with the current implementation. We should add a comment about this though.
I wasn't sure if it would be best to patch hiprt to not include the version in the name (since that is quite non-standard), or if we needed the extra code complexity to deal with the version. But I guess we can do neither.
yeah we can ignore the .lib if it's not being used, it was being harvested due to
${SHAREDLIBEXT}being.libon windows, since for linkage we need to link the import library, and not the shared lib it self. For harvesting we just need to harvest the actual dll, and we can give it any name we want. The issue is a bitSHAREDLIBEXTis being used for something it wasn't meant for, maybe better to do something like this.dll.lib.dylib.dylib.so.sobut that'll require changing quite a few files of the deps builder and with quite a few PR's in flight currently, i'd really rather not do that right now, for now think we can just hardcode the harvest to use .dll i'll get that done later this week.
I think we can do away with hiprt version. The version is relevant, if the bitcode is supplied by the SDK.
@brecht
HIP-RT has been updated and now the changes are public: https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/releases
Let me know if there is any pending task from AMD side.
@salipour It seems this added CMake support, but most of the reasons for why I needed these patches still remain. For references these are the patches:
2ee2604f1f/build_files/build_environment/patches/hiprt.diffHIP_PATHon Linux. Also would be better as a cmake variable instead of environment variable.easy-encryptionis not in the release package, but if we build without we don't need it)pythoncommand is available, but often there is onlypython3on Linux.make installto install files.Other issues found:
tools/bakeKernel.sh: 2: cannot create hiprt/cache/Kernels.h: Directory nonexistent. There is a wrong assumption about current working directory, it should be set explicitly inCMakeLists.txt.easy-encription is no longer needed. Thanks for the feedback. We'll update HIP-RT and let you know.
ideally there wouldn't be any writing in the source dir, so making a cache folder there is a bit suspicious
Thanks. I got build error due to missing easy-encryption headers, and missing binary in the Python kernel baking code.
For reference these are the changes I did to change to CMake, but I didn't push to the branch as it's not working for reasons mentioned.
Diff
I meant make install in the hiprt project. We can manually copy files from the dist folder too, it's not a big deal, just unusual for a cmake project not to have an install target where the target can be configured with
CMAKE_INSTALL_PREFIX@brecht
My colleague, Richard, updated the HIP-RT repo and hopefully now the setup aligns with your expectations.
Here are the latest updates:
New release package:
https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/releases/tag/2.3.bd75b7c.rc4
cmake script updated:
you can specify the HIP_PATH
you can build without easy-encryption (NO_ENCRYPT) but you probably want to leave easy encryption on until we fully test this option
you can build without unit test ( NO_UNITTEST )
you can specify the python command ( use env var: PYTHON_BIN )
there is a make install
Currently, Blender first looks for ROCM 5.x runtime (amdhip64.dll), then if the dll is not found, it checks ROCM 6.x (amdhip64_6.dll). HIP-RT order is the other way around. I still prefer to use the un-versioned dll since it's a tiny bit faster. In order to have HIP-RT load the un-versioned dll, you have to pass -DHIPRT_PREFER_HIP_5=ON as well.
example of command in Linux :
export PYTHON_BIN=python3
mkdir build
cmake -DCMAKE_BUILD_TYPE=Release -DBITCODE=ON -DNO_UNITTEST=ON -DHIP_PATH="/opt/rocm" -DPRECOMPILE=ON -DHIPRT_PREFER_HIP_5=ON -S . -B build
cmake --build build --config Release
cmake --install build
It's building ok now on Linux. Though I have not tested with an AMD GPU to check if it actually works.
I still had to work around two issues, but it could be done without patching so it's not too bad. Not sure if Windows will have a similar permission issue.
Yes, chmod +x ./contrib/easy-encryption/bin/linux/ee64 is needed because ee64 is used by the script during kernel pre-compilation.
@LazyDodo @Sergey Please let me know if there is anything on AMD side that needs to be done before you can move forward. I really want to make sure this change is merged in time to pass all 4.3 check points. I still have to upload some changes for HIP-RT kernels to compile properly but want to wait until you sign off on the folder structure in lib folder for both Linux and Windows and HIP-RT binaries/headers are available for download on your repo.
Sergey takes over, but gita interface is not good
@salipour Hi. The folder structure seems fine to me. I wouldn't worry too much about it. It's easy to tweak at any time.
I think the plan here would be:
@Sergey Can I upload the libraries to the repositories myself? I built the dependencies on Linux, but I wasn't able to do the same on Windows.
@LazyDodo what do you think? Should I create a new pull request? The current pull request points to the hypothetical binaries/headers but it requires some refinement.
I Haven't looked at this in a while, so unsure about the exact state of this PR, I'll try to schedule in some time this week to take a closer look.
That being said what would a new PR bring to the table? Are there compelling reasons the issue cannot/should not be fixed in this PR?
@LazyDodo There are some minor changes, nothing fundamental. And, the changes are related to the functionality of Blender build when it picks up the HIP-RT headers and binaries from third party lib folder. For example, in one of the make files, "orochi" is used, instead of "Orochi."
@salipour I am not really sure submitting libraries will speed things up here. We still need to ensure the build system properly compiles all libraries.
Did you compile those libraries on Rocky 8?
And for the Windows, is it you didn't have access to the platform yet, or are there some issues there?
@ -0,0 +5,4 @@# Note the utility apps may use png/tiff/gif system libraries, but the# library itself does not depend on them, so should give no problems.if(NOT (BUILD_MODE STREQUAL Release))Projects not building for Debug is generally handled in the main CMakeLists.txt
@ -0,0 +9,4 @@return()endif()set(HIPRT_CMAKE_FLAGS ${DEFAULT_CMAKE_FLAGS})This isn't used anywhere
@ -0,0 +46,4 @@PREFIX ${BUILD_DIR}/hiprtINSTALL_DIR ${LIBDIR}/hiprtCONFIGURE_COMMANDby setting just
CONFIGURE_COMMANDand notBUILD_COMMANDcmake will just callmaketo build the project which is problematic since thePLATFORM_ALT_GENERATORis set to ninja on windows.@ -0,0 +47,4 @@INSTALL_DIR ${LIBDIR}/hiprtCONFIGURE_COMMAND${HIPRT_WORKAROUND}The marking of the script as executable can be handled using the
PATCH_COMMANDas it only needs to be executed once.@ -0,0 +48,4 @@CONFIGURE_COMMAND${HIPRT_WORKAROUND}PYTHON_BIN=${PYTHON_BINARY}This doesn't work on windows, it requires
set PYTHON_BIN=${PYTHON_BINARY}this probably be fixed by sticking it in theHIPRT_WORKAROUNDsection above however since that has been moved to a patch step and....Further complications, hiprt just calls python and assumes it is in the path over here this still does not work on windows as there is still no system python. It be best if hiprt used the FindPython3 module to locate a suitable python interpreter and propagate the env var themselves to bakekernel.bat/sh or skip the batch file all together and just do the work from cmake.
With the env var now no longer being needed, and the workaround now being handled in the patch step, this whole section can now revert to using regular
CMAKE_ARGSwhere cmake will call the right build tool on its own.Actually, PRECOMPILE=OFF will work fine. compile.py script generates the binaries and bitcodes. Now that HIP-RT is open-source, Cycles can directly build the binaries here
I will remove PRECOMPILE from hiprt.cmake. That should resolve all the complications about Python?
I'm afraid not, given we also build with
BITCODE=ONit'll still call bakeKernel and run into python issues there.Right....
bakeKernel is also redundant. I will remove all the leftover from the closed source era and open a pull request myself.
with
BITCODE=OFFit appears to build fine with no further python issues, it warns about our cuda versionwe're currently standardized on 12.3 but it doesn't appear to be causing build failures
It won't cause build failure, but it will fail at runtime if BITCODE is not enabled.
@ -0,0 +63,4 @@${LIBDIR}/hiprt/bin/hiprt${HIPRT_LIBRARY_VERSION}64.dll ${LIBDIR}/hiprt/bin/hiprt64.dllDEPENDEES install)ExternalProject_Add_Step(external_hiprt after_installOnly a single
after_installis allowed, these two will need to be merge or cmake will error out.@Sergey
I compiled all the libraries on Ubuntu 22.04. I had to install some other packages along the way, but nothing related to building HIP-RT. Should I try on Rocky? I haven't worked with it before, but I can try if that makes the process easier.
On Windows, which is my main dev environment, "make deps" is not recognized.
This is Brecht's response regarding building dependencies and open sourcing HIP-RT:
I can start with build_deps.cmd and see how it goes.
Windows and linux are fundamentally not all that different, except for the make helper, couple of pointers here though:
we build the deps on vs 2019 - v16.11.26 , it has seen no other more modern versions, you can try to use them, but this is untested, and unsupported.
Windows has Long path issues and things just break there,, my working folder for the deps builder is c:\db for that reason
a successful build of all deps takes about 12H on my environment, given hiprt is pretty standalone, you can likely skip most of them by just building the hiprt subproject, once build_deps.cmd prepares the build env and starts building the actual deps, kill it, and run the following batch file in
c:\dbto just build hiprt (and and its dependencies)
@LazyDodo Thank you for the instruction to build the dependencies on Windows; it was very helpful.
I can confirm HIP-RT builds successfully on Windows and Linux (Ubuntu.)
I still have to change BITCODE option back to ON (in hiprt.cmake) after HIP-RT changes make it to the public repo.
In the meantime, could you review the code and let me know if there were any concerns?
45d238b1e0to30cdfdf642There seems to be something wrong went with the merge conflict resolution.
The harvesting part needs to be revisited, so it is aligned with what is doing #123196. At its current state the PR is not usable on Linux: there are a lot of CMake errors reported. If you can ensure it works on your side that'd definitely be of a great help and make it easier to move forward.
To replicate the Linux environment:
Minimal Installconfiguration.sudo sh ./build_files/build_environment/linux/linux_rocky8_setup.sh.~/.bash_profile:./build_files/utils/make_update.py --use-linux-libraries).make deps.I've verified the steps a few days ago with rev
471a148666, and the dependencies compiled correctly. Ideally the proposed PR would also work on this environment.30cdfdf642to68d52dd786My bad! I shouldn't have pushed the rebase before rebuilding dependencies. The harvest.cmake file, as you pointed out, was incorrectly merged. I fixed it and am building the dependencies on Ubuntu and Windows.
On another note, I had some issues with FindHIP module in finding HIP_LINKER_EXECUTABLE on Windows.
Without NO_DEFAULT_PATH the modules picks the clang++ from SYCL, so the flag has to stay. But with it, find_program() can't find the linker that is under HIP_PATH/bin.
If I limit the _hip_SEARCH_DIRS to only HIP_ROOT_DIR on Windows, then the linker is found. So, locally FindHIP looks like this for me
EDIT: The links are not showing in the comment.
HIP_LINKER_EXECUTABLE :
_hip_SEARCH_DIRS :
@Sergey
I didn't run the dependency building to the completion but many of the dependencies, including hiprt, were successfully built by the time I ended the build.
I will try to build on Rocky as well; but at least Ubuntu seems to be in good shape.
bdad2a737bto33f1ea47f133f1ea47f1to4eb3743881@Sergey and @LazyDodo
I think everything is in good shape. I updated the version file to point to the latest version of HIP-RT that removes the Python dependency and confirm dependencies build is successful on Windows.
Rocky is still going on. But HIP-RT build is done and was successful.
Blender build with HIP-RT enabled is successful and functional. I have, however, experienced random crash with both HIP and HIP-RT and don't know yet whether it is related to rebasing to main or AMD driver or my system's instability.
Side note:
I had to disabled OpenVDB and projects that depended on it (USD and MaterialX.) OpenVDB required gcc version 9+ but after removing the entire tool set and reinstalling version 11 and running the rocky script, I would still get version 8.5 ¯(ツ)/¯
I also disabled LLVM build and all the projects that depend on it.
Please let me know the next step to finalize and complete this pull request.
EDIT: just in!
Dependencies successfully built and installed to /root/AMD_HIPRT/lib/linux_x64.
did you activate the toolset?
Thanks for the updates. I was able to compile hiprt library on Linux without a major issue. Currently testing the Blender build, which could take a bit. Meanwhile some comments based on reading the PR.
The
version.txtwas missing from install, not sure what's up with that. Maybe some harvest mask is wrong or something. I wouldn't worry about it, as the functionality of it needs to be taken care elsewhere. See the inlined comment about it.There are also comments in some other areas of the code, which also needs to be addressed.
@ -711,3 +709,1 @@option(WITH_CYCLES_DEVICE_HIPRT "Enable Cycles AMD HIPRT support" OFF)mark_as_advanced(WITH_CYCLES_DEVICE_HIPRT)endif()option(WITH_CYCLES_DEVICE_HIPRT "Enable Cycles AMD HIPRT support" OFF)Indentation seems to be broken.
@ -122,2 +122,4 @@endforeach()")endfunction()harvest(alembic/include alembic/include "*.h")This change effectively reverts change made in #123196.
The harvesting is to be done in the individual files.
Not sure what's going on here. Looks fine in the branch on its ownhttps://projects.blender.org/salipour/AMD_HIPRT/src/branch/HIPRT_OPEN_SOURCE/build_files/build_environment/cmake/harvest.cmake
@ -41,0 +44,4 @@hiprt_device_->global_stack_buffer);if (rt_result != hiprtSuccess) {string_printf("Failed to create hiprt Global Stack Buffer");string_printfis a portable version ofsprintf(), it doesn't print anything to the console. We should probably make it to warn if the result is unused to help catching issues like this.Use
LOG(ERROR) << "Failed to create hiprt Global Stack Buffer";to report errors to the console.@ -29,3 +26,1 @@FLOAT32,KERNEL_FILM_CONVERT,};enum Type { POINTER, INT32, FLOAT32, KERNEL_FILM_CONVERT, HIPRT_GLOBAL_STACK };Use trailing
,after value, so that there is a single value per line, making it more obvious what is added/removed.@ -745,3 +836,3 @@if(WIN32)set(hiprt_link_command ${CMAKE_COMMAND})set(hiprt_link_flags -E env "HIP_PATH=${HIP_ROOT_DIR}"If there are no funcitonal changes please avoid style changes. Makes it easier to focus on actual functional changes.
@ -697,2 +698,3 @@${SRC_UTIL_HEADERS})set(bitcode_file ${CMAKE_CURRENT_BINARY_DIR}/kernel_rt_gfx.bc)# Get HIP-RT versionfile(STRINGS ${HIPRT_INCLUDE_DIR}/../version.txt lines)For some reason
version.txtwas not copied to thelib/linux_x64/hiprtfolder, so this part of the code couldn't have tested.It also hardcodes compilation to a specific structure of percompiled libraries folder, making it harder to fit into Linux distros which ship Blender and don't use our precompiled libraries.
Such detection is to happen in
FindHIPRT.cmake, based on thehiprt.h.An example of this is
SYCL_VERSIONinFindSYCL.cmake, andOSL_LIBRARY_VERSION_MAJORinFindOSL.cmake.It worked for me on both Ubuntu and Windows. I will double check.
Anyway, I will do away with version.txt and use the header to get the version instead for both Windows and Linux.
@Sergey can you explain a bit more about how this doesn't fit nicely into Linux distribution? Does it mean that shaders are not compiled and will be compiled the first time Cycles is run?
Many Linux distros are providing their own packages of Blender, with the dependencies from their repository, and do not use our pre-compiled libraries.
It is likely that Debian/Ubuntu/Gentoo will want to compile HIP-RT as a re-usable package, similarly how they deal with all the other dependencies (like, nvcc, oneapi/dpc++, libraries, etc). This means that the Blender build process can not rely on the way how some dependencies are apckaged.
@ -708,12 +719,79 @@ if(WITH_CYCLES_DEVICE_HIPRT AND WITH_CYCLES_HIP_BINARIES)else()set(hiprt_compile_command ${HIP_HIPCC_EXECUTABLE})set(hiprt_compile_flags)#set(hiprt_compile_flags -mcode-object-version=4)Remove dead code.
@ -761,0 +861,4 @@if(WIN32)set(BVH_INSTALL_PATH ${CMAKE_INSTALL_PREFIX}/blender.shared)else()set(BVH_INSTALL_PATH ${CMAKE_INSTALL_PREFIX}/lib)Why does this go to
${CMAKE_INSTALL_PREFIX}/blender.shared/${CMAKE_INSTALL_PREFIX}/liband not to${CYCLES_INSTALL_PATH}/lib?Using
blender.sharedhere would break Cycles standalone.@Sergey is correct, but just as an educational opportunity for likely both, you can't install into
blender.sharedlike that, it's governed by an manifest that keeps track what files are in that folder, if a shared lib is not listed in it, it will not be found by the OS loader.windows_install_shared_manifestMUST be used for all files installed there.These two files are loaded by hiprt64.dll, so they have to be copied somewhere that hiprt64.dll can find them.
@Sergey how should Cycles standalone be handled. The binaries should go wherever hiprt.dll goes. That also means for Linux without precompiled binaries, these binaries should get built and copied to where hiprt.so can see them. What do you recommend?
@Sergey @LazyDodo
What do you think about the location of the couple binaries discussed here? They have to be seen by hiprt library. Cycle standalone? Linux first time compilation? Let me know what you think, and I can update the code accordingly.
Something that does not feel fully ideal, but matches what we do for oneAPI AoT kernels would work:
As Ray was mentioning
blender.sharedshould not be used directly.oneAPI doesn't do it either. While it is a bit unideal to store libraries in the root of the application folder, it is known to work for now and for the ease of moving forward I'd suggest sticking to something we know is working for other backends.
This is blender standalone specific, for mainline blender, yes, it be un-ideal to have more shared libs next to blender than required, (it's the main reason blender.shared exists, people complained there was too much "stuff" in the main blender folder) for standalone, for me personally at least that's not a concern, right by the binary is fine to me.
Right now, this is how it is laid out:
Cycles stand-alone: hiprt64.dll and shader binaries (hipfb files) are in the root directory
Blender full build: hiprt64.dll and shader binaries are in the blender.shared folder.
Is it acceptable as is? Or do you prefer moving the files to the root directory for Blender full build as well?
Since shader binaries are loaded by hiprt64.dll and not by the OS loader, is it okay not to use windows_install_shared_manifest?
And, for possible future improvement, what will an ideal solution look like?
There is a bit of a difference between the final layout, and where/how CMake rules are written for this. There also seems to be a level of confusion about precompiled libraries, and the BVH libraries.
The
hiprt64.dllis to be installed intoblender.sharedusingwindows_install_shared_manifest. This is already handled in thesource/creator/CMakeLists.txtin this version of PR. It is probably fine, although not fully clear when something is to be handled via explicitly there, and what is to go viaPLATFORM_BUNDLED_LIBRARIES. The latter one seems more modern way of how we handle bundling, but hearing from Ray would be good.In any case no further handling of
blender.sharedis to happen from Cycles.On linux the
hiprt.sois handled byadd_bundled_libraries(hiprt/lib)inplatform_unix.cmake, so that's also already taken care of.For the Cycles standalone bundling of such libraries is done by a CMake rules which is not currently in the main Blender repo. That we'd need to take care of when we'll be porting changes to the standalone repository. It is not something that seems possible to handle as this PR.
The installation of
${bvh_file}and${bvh_file_oro}(which, I believe, part of the HWRT that is compiled from kernel sources) is to follow the same rules as currentcycles_kernel_oneapi_lib(which is the closest similar thing we already do with the other backend). I wrote details about it in the previous comment.afaik
PLATFORM_BUNDLED_LIBRARIESisn't used on windows, it lacks the functionality to install different things depending on debug/release builds, we could likely make it work with some macro magic but as of right now this is not the case and isn't used (that i'm aware of)@Sergey @LazyDodo any comment on the pending issues?
@Sergey I updated the install path as well. Please let me know of any more concerns. I am eager to get this pull request merged.
WIP: Linux Support for HIP-RTto Cycles: Linux Support for HIP-RTCompilation part
Building dependencies are successful, so that's good. Although, is seemingly incomplete.
Building Blender with HIP-RT enabled fails compilation:
Either something is missing in build configuration, or harvesting is missing.
If you start with a clean Linux environment (as described in some earlier comment) and clean checkout, this should work:
main branchmake depsmake CMAKE_CONFIG_ARGS='-DWITH_CYCLES_DEVICE_HIPRT=ON' releaseYou shouldn't be needing to copy any files manually to the libraries folder. It should be handled by the dependency builder (during the
make depsstep).Installation part
For the installation steps we can either simply copy the same logic as is done around
delayed_install("" "${cycles_kernel_oneapi_lib}" ${cycles_oneapi_target_path})(including thecycles_oneapi_target_pathpart). Or, more ideally, move theif(NOT WITH_BLENDER)closer to the beginning of the file (making it reusable), and haveAnd then oneAPI case becomes:
delayed_install("" "${cycles_kernel_runtime_lib_target_path}" ${cycles_kernel_runtime_lib_target_path})And the HIP-RT:
@ -769,0 +855,4 @@if(NOT WITH_BLENDER)set(BVH_INSTALL_PATH ${CYCLES_INSTALL_PATH})set(HIPRT_LIB_PATH ${LIBDIR}/hiprt/bin)delayed_install("${HIPRT_LIB_PATH}" "hiprt64.dll" ${CYCLES_INSTALL_PATH})Precompiled libraries for Cycles standalone are handled outside of this file. There should be no need to have this special case here.
We'd need to handle it in the
external_libs.cmakeof the standalone repo when we'll be porting the change there.I am not sure where to move this part. I was falling back to the same logic as One API:
Also, I guess I have to remove this part.
@Sergey were you referring to this https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/cmake/external_libs.cmake
There are just few extra things that happen here and seeming should not happen.
From the fact that
bvh_fileandbvh_file_orowere initially targeted to be installed to alibsfolder I've assumed they are dynamic libraries and should be handled something like this:However, they are not a dynamic library, but a data of some sort. So from this perspective they seem to belong to the same place as the rest of the precompiled kernels (cubins, fatbins, and the
kernel_rt_gfx.hipfb.zst).Where exactly those these
bvh_fileandbvh_file_oro(hiprt02003_6.1_amd.hipfbandoro_compiled_kernels.hipfb) are used during runtime?bvh_file and bvh_file_oro are loaded by hiprt64.dll. If moved to where rest of the precompiled kernels are, hiprt64 will not be able to load them.
Here is when they are used:
and
@ -769,0 +864,4 @@endif()delayed_install("${CMAKE_CURRENT_BINARY_DIR}" "${bvh_file}" ${BVH_INSTALL_PATH})delayed_install("${CMAKE_CURRENT_BINARY_DIR}" "${bvh_file_oro}" ${BVH_INSTALL_PATH})Indentation is broken.
No WITH_CYCLES_HIP_BINARIES?
The
WITH_CYCLES_HIP_BINARIESare taken care by the release configuration (in theblender_release.cmake).The issue can be seen before building Blender: the
make depsdoes not place thehiprt_kernels_bitcode.hanywhere in thelib/linux_x64/hiprt. The hiprt libraries and some headers are copied there, bit thehiprt_kernels_bitcode.his missing.This is something me and Ray quickly talked about: it seems on Windows files are not copied automatically into the final location, while they do seem to copy on macOS and Linux. In any case, the
hiprt_kernels_bitcode.his not found in neither under./lib/linux_x64/hiprtsubmodule, nor inbuild_linux/deps_x64/Release/hiprt.So some harvesting or copying seems to be missing.
Yea, it has to do with the harvesting. I had tested HIP-RT build on Ubuntu before but not after #123196.
Actually, today I've noticed something strange: the
releasepart of themakecommand did not properly pick up the release CMake configuration.It is nothing to do with this PR, and I didn't see it happening before. So we'd need to keep an eye when testing things with such one-liners.
Ah, cool. The updated
harvest.cmakeseems to do the proper thing now.Think I will leave the
make depspart of the review up to the maintainers at this pont. Its their domain of expertise.I've merged the latest
maininto the branch, and solved some various cosmetic issues. General notes for the future:The things that need attention from my side:
bvh_fileandbvh_file_oro(hiprt02003_6.1_amd.hipfbandoro_compiled_kernels.hipfb). They do not seem to belong to the libs folder, but it is also unclear where they are used form.@ -143,0 +144,4 @@string source_path = path_get("source");/* Set HIPRT_PATH env for BVH kernels. */string hiprt_path_runtime = "HIPRT_PATH=" + path_join(source_path, "kernel//device//hiprt");Double-slashes, should it be
kernel/device/hiprt?@ -143,0 +145,4 @@/* Set HIPRT_PATH env for BVH kernels. */string hiprt_path_runtime = "HIPRT_PATH=" + path_join(source_path, "kernel//device//hiprt");char *hiprt_path_env = const_cast<char *>(hiprt_path_runtime.c_str());No need to create temp variables just for casts.
Hey sorry didn't mean for this change list to get reviewed. I was moving from Linux to Windows so, I pushed the changes to pick up on Windows. Should have pushed to a different branch!
@ -143,0 +146,4 @@/* Set HIPRT_PATH env for BVH kernels. */string hiprt_path_runtime = "HIPRT_PATH=" + path_join(source_path, "kernel//device//hiprt");char *hiprt_path_env = const_cast<char *>(hiprt_path_runtime.c_str());putenv(hiprt_path_env);I don't really understand where this is uses. The only access to the
HIPRT_PATHenvironment variable I see is few lines below. What parts of code actually depend on it?If it's only used here in the device, no need to set an environment variable.
If it's somehow used by other parts of our code then:
HIPRT_PATHsounds like a path to HIP-RT SDK, and not to our kernel code.@Sergey sorry didn't mean for this change list to get reviewed. I was moving from Linux to Windows so I pushed the changes to pick up on Windows. Should have pushed to a different branch!
That was actually the idea. To set HIPRT_PATH if HIPRT_SDK wasn't already set.
I was trying to move compilation of bvh_file and bvh_file_oro to first time the app is launched, since these two files and when they are placed have been the subject of a lot of discussion.
The way it works inside hiprt library is that hiprt looks for bvh_file and bvh_file_oro and if they don't exist, it compiled them from source.
There are a number of issues with that approach:
1- The end user should have HIP SDK/compiler installed for that to work
2- This part is supposed to happen under the hood and an application using the library doesnt need to know about what is happening, so it shouldn't have to set the PATH either
3- Plus, even if that was an acceptable solution, the environment variable should get set before hiprt64 is loaded, so not in this section of the code.
I admit the limitation where to place bvh_file and bvh_file_oro is not ideal and borderline breaks the patterns in Blender but right now putting hiprt64 and the two bvh shader binaries in the same location (e.g. root directory) is the only option I can think of.
@Sergey what do you think about the placement of these files? I have them all in the root directory. I have built the dependencies and the app on both Windows and Ubuntu. It is functional with the current version of the code.
@ -218,3 +225,3 @@double starttime = time_dt();const string hiprt_path = getenv("HIPRT_ROOT_DIR");const string hiprt_path = getenv("HIPRT_PATH");There is no need to get an environment variable right after it was set. Just use the
std::stringvariable from above (thehiprt_path_runtimebut withoutHIPRT_PATH=part).@salipour Can you write a small design summary, making it more clear what are the moving parts, and when things are supposed to happen (AoT, JIT, etc)? It is not something that is 100% clean, and things seem to go back-n-forth in the code, which does not help understanding what's going on.
On top of that, it would be very nice if the updates to the PR were more descriptive. Some of the recent commits seem to include unrelated changes. For example, the
Updated the copy location of bvh kernel binariesincludes changes to the way how the library is built (-DBITCODE=ON), without mentioning this.Keeping in mind previous conversation about this makes it extra unclear: some messages seem to suggest bitcode is not needed, but then that it actually is needed because of runtime, but also there are some compilation errors mentioned related to Python and BITCODE. It is very confusing.
I don't really understand why those files could not provided explicitly via API. Similarly how render kernel does not need to follow any naming convention, and does not need to be in any specific location w.r.t to driver or other libraries.
@ -239,0 +259,4 @@rtc_options.append(" -x hip");rtc_options.append(" -D HIPRT_BITCODE_LINKING ");string source_path = path_join(hiprt_include_path, "//hiprt//impl//hiprt_kernels_bitcode.h");Double slashes.
@ -717,0 +805,4 @@delayed_install("" "${bvh_file}" ${cycles_kernel_runtime_lib_target_path})delayed_install("" "${bvh_file_oro}" ${cycles_kernel_runtime_lib_target_path})delayed_install("${hiprt_lib_path}" "${hiprt_lib}" ${cycles_kernel_runtime_lib_target_path})Please remove handling of
libhiprt64.soandhiprt64.dllfrom this file.For the purpose of this PR, Linux is handled via
PLATFORM_BUNDLED_LIBRARIES(hiprt is added viaadd_bundled_library(hiprt/lib)). Windows is to be handled by theCMakeLists.txt. Some recent change removed that code.The Cycles standalone we'll handle when we'll be porting the changes there. The important thing here is to just write code in a way that will not cause problems for that porting process. If you follow what I've write then updating the standalone repository will be easy.
@ -717,0 +822,4 @@set(hiprt_kernel_src "/device/hiprt/kernel.cpp")set(hiprt_compile_flags_sdk_bc${hiprt_compile_flags}Indentation.
@Sergey
The explanations for the different parts are scattered across various threads, so I'll do my best to consolidate everything here.
Will do. I added it temporarily until the kernel binaries placement is sorted out (more on this at the end of the comments)
I was trying to place hiprt in the same location as the kernel binaries. I removed the installation of hiprt64.dll from the CMake file because it would place hiprt64.dll in the blender.shared. So, I must adjust the CMake to copy hiprt64 it to the Blender root directory instead. The way hiprt64 should be handled is quite confusing for me (and probably self inflicted). I want to make the code changes in the right place, but I also want to ensure that hiprt64.dll/so, hiprt02003_6.1_amd.hipfb, and oro_compiled_kernels.hipfb all end up in the same location.
I thought I already did that. Can you clarify what part of the instruction you are referring to?
BITCODE has always been enabled. Earlier this week, when transferring my work from Linux to Windows, I mistakenly pushed changes into this branch. Please disregard any changes made in that commit.
a73c214129BITCODE allows HIP-RT to rely on precompiled binaries instead of compiling and caching them on first use. I briefly considered runtime compilation to eliminate the need for kernel binaries, but this would require users to have the HIP SDK installed.
Python is not needed when BITCODE is enabled. HIP-RT was originally configured to load either precompiled kernels or encrypted kernel sources, with the latter requiring Python. But, these two components are now decoupled.
HIP-RT provides traversal and intersection functions, as well as BVH construction kernels. When HIP-RT was closed-source, the intersection functions were provided in the HIP-RT SDK as a precompiled library, which would be linked against the Cycles kernels at either build time or runtime. BVH kernels and the DLL were distributed through the AMD driver.
Now that the code is open-source, both the compilation and linking of intersection functions take place within Cycles. However, the process introduces handling of BVH kernels—hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb—which are executed when hiprtBuildGeometry and hiprtBuildScene are called:
For end-users, these kernels must be compiled ahead of time. JIT compilation could be an option for developers but it requires the HIP-RT SDK to be installed, the HIPRT_PATH environment variable to be set, and the same HIP SDK version that compiled hiprt64 to be used. The last requirement makes the process too fragile and made me change the code to always compile hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb ahead of time.
I hope the comments so far made things clearer. If not, let me know and I will add more context.
Once everything
Once you think everything is functionally correct and in line with Blender design, I will do a final check for cosmetic issues, dead or redundant code.
I agree with your assessment. HIP-RT was open-sourced without any changes to its original closed-source design, which was meant to keep the implementation of BVH construction and traversal methods secure and work around the absence of an intermediate format.
This project is owned by another team, and the turnaround time for code change/review and updating unit tests is slow. This delay could potentially extend beyond the deadline for merging this pull request in time for the 4.3 release. I appreciate that this isn’t your issue, and I understand that even temporarily placing the kernels (hiprt02003_6.1_amd.hipfb and oro_compiled_kernels.hipfb) in the same location as hiprt64 conflicts with Blender’s design principles.
To address this, I created a fork of HIP-RT and added an option to bundle the kernel binaries into the hiprt library. I hope to eventually merge this change into the main repository.
I’d like to know your thoughts on using my HIP-RT fork to resolve this issue. The fork is linked to my AMD account, so in the event that I’m unavailable (e.g., run over by a bus), someone else could take over. This seems like a viable solution given the tight timeline. Alternatively, would you consider merging the pull request as it is and keeping the kernel binaries in their current, though suboptimal, location? If so, I’ll work with the HIP-RT team to ensure the bundling option is integrated into the main repository before the November release.
@Sergey please let me know if my previous comments clear up confusion and also let me know how you would like to move forward with BVH binaries (do you want to use my HIP-RT fork?)
Sergey Sharybin referenced this pull request2024-08-19 16:11:46 +02:00
@Sergey what's the next step? I am waiting for your decision about the bvh binary files.
@salipour Thanks for the explanation and updates. Feels like we're getting somewhere.
My plan is the following:
Assuming I'll be using ubuntu 22.04 or 24.04 on the test machine, what is the driver version I'd need to have for tests?
I tried testing this PR on Windows and it doesn't work. My testing points to it being related to
hiprt64.dllnot being installed anywhere thathiprtewInit()can find it.Is this expected to be the case?
@Alaska Is a bit unclear whether you've run the
make depstype of a thing. I assume you did :)One of the obvious thing now when you mention it, we need to restore this part of the code:
7358de3ee8 (diff-e165d5b8de6cfd61c8a0c9b1d5b223f50c55c9f7)The
hiprt64.dllneeds to be installed by the CMakeLists.txt in the creator.But, I am not 100% sure the LoadLibrary type of approach follows the manifest. Hopefully it does.
Sorry I should of explain that better.
I ran
make deps, picked out the HIP-RT library from that and put it in my libs folder. Then created a new build of Blender.The issue I observed was that HIP-RT was greyed out in preferences. So I tracked down the code to
hiprtewInit(), which was returning false (telling Cycles that the GPU didn't support HIPRT) because it couldn't findhiprt64.dllI just manually copied
hiprt64.dllto beside the Blender executable and that fixed the issue and allowed me to move on to testing. And I can confirm it is actually HIPRT as it shows some HIPRT specific artifacts I've seen in the past, and the artifacts disappear switching back to BVH2.Another thing I noticed while building was that
makewith a configuration belowreleasefailed. E.g.make nobuildThe error seems to occur because
HIPRT_INCLUDE_DIRisn't defined yet (becauseWITH_CYCLES_DEVICE_HIPRTisn't enabled in this configuration).A quick recap from testing on Windows with a RX 7800XT.
Running automated render tests and benchmarking, there are no performance, memory, or render regressions that I can find. And in fact performance is improved in some scenes and some issues have been fixed.
I have not tested viewport stability/reliability, and Linux obviously still needs testing.
Ah! The check for WITH_HIPRT is left behind on my local system.
@Alaska did you copy hiprt64.dll manually? How did you make it work? I have to update the code to copy the dll to the root directory.
@sergey any recent driver should work.
When it wasn't working I hadn't copied it manually. But later on I copied it manually and used that for my regression and performance testing.
@Sergey for
e4ae1b5533I didbuild the dependencies on Rocky 8 and Ubuntu.
build Blender with HIP-RT enabled on Ubuntu and tested rendering.
build Blender with HIP-RT enabled on Windows as well and briefly tested rendering.
Our QA will do extensive testing once the daily builds become available for this PR.
Let me know if you have any concerns.
@salipour Thanks for the updates.
I've been testing the patch for building libraries on Rocky 8, and Blender on Ubuntu 22.04. It seems to work with a bit of caveats I'll come back to in a moment. The render time on Junkshop dropped from 22 seconds to 16 on WX7800. That's nice :)
The issues I've run into:
kernel_rt_gfx.hipfbtakes about 30min to compile with rocm 6.1.2 and 6.1.3 (the latter one is what came with the driver on Ubuntu). That's a bit unideal, as it risks stalling our CI/CD pipeline. There was some discussion earlier on, and 6.1.1 seemed to work. Can you confirm the compilation times on your end?At this point I wouldn't mind getting extra eyes from the platform maintainers (@LazyDodo and @ideasman42) to do the review.
More or less the same for me (although I have a very old cpu on my Ubuntu machine.) We can mitigate it by some changes on the HIP-RT library to have compile and link in one step but for the moment, I will break down the compilation into individual binaries per architecture in the CMake file. It will probably add up to the same amount of time, but no single compilation task will take more than couple of minutes then.
For me the timing is about 16min on i9-11900K, but it goes up to 30min when running in a VM. This only includes compilation time
kernel_rt_gfx.hiprt. The configuration of similar CPU + VM depicts the hardware used on the buildbot quite well. So it is a bit worrying to see such step added. The time is forkernel_rt_gfx.bcandkernel_rt_gfx.hiprtbtw.Do you mean we can break down the
kernel_rt_gfxinto individual architectures?Is there a flag we can pass to the HIP compiler to build more things in parallel at a cost of extra memory use (similar to
-fsycl-max-parallel-link-jobs) ?Having more smaller tasks running in parallel should help preventing situation when one big task is compiled at the end of a build process and clogs the entire pipeline (we don't exactly have a way to control order of kernels to be compiled).
Yes.
Yes, -parallel-jobs should do it.
Update::
@Sergey the change will look like something like
aa54a52051I compiled and run Blender on Ubuntu, and while it works without a problem, something is not right. The hipfb files keep growing in size as if later binaries in the list are built for multiple targets. I have to go over the changes in the cmake more carefully .
@salipour I had a quick look into the
-parallel-jobs. The results are quire cool, actually. With 4 parallel jobs the compilation time toughly 1/4, and (unless i've missed something) the memory usage stays within reasonable amounts.I've attached the patch for reference.
Not sure it worth going more complicated route with splitting tasks on CMake level if we can use a simpler solution.
I think it would be fine to add an option similar to the
SYCL_OFFLINE_COMPILER_PARALLEL_JOBS:And then whenever applicable, to
-parallel-jobs=${HIPRT_COMPILER_PARALLEL_JOBS}For the buildbot we'll set the value from the build pipeline configuration, and keep compile time manageable.
Good! If this option reliably works, then I'd much rather stick with the way it is set up now, rather than splitting the work like I did in the change list I referenced earlier.
@salipour I've committed the
-parallel-jobsto the PR, as well as some tweaks to the CI/CD configuration.I don't think I have stoppers from my side now.
There are a few things I'd like to simplify in Cycles w.r.t the way we handle kernels and such, but it should happen for all platforms, and after this PR has landed.
We still need to ensure platform maintainers reviewed the change, and possibly sync-up for the library updates.
Thank you @Sergey
I'm glad the PR is now in a reasonable shape.
@LazyDodo and @ideasman42 let me know what you think.
I will be away until next Tuesday. If there are any concern, I will address it after I return.
@ -0,0 +44,4 @@${LIBDIR}/hiprt${HARVEST_TARGET}/hiprtCOMMAND ${CMAKE_COMMAND} -E copy_directorythis causes a whole bunch of .cpp files inside the final libdir, the root cause is when running the install phase of the hiprt project it does not install any of its own headers and
build_files\build_environment\cmake\hiprt.cmakeends up pillaging the source folder to get them. this is not how that is supposed to work. This needs to be fixed in upstream hiprt.I will add this (hiprt not installing any of its headers) to the list of changes I am compiling to hand over to the hiprt team. The project does install some of its headers (in the dist folder) but only the ones required in a closed-source setup.
@LazyDodo
HIP-RT now handles installation of its own headers and the change is public. I updated the CMake files to pick up the latest version of HIP-RT and removed unnecessary copies
db7dc075bcLet me know if everything looks okay now.
@ -55,1 +49,3 @@HIPRT_INCLUDE_DIR HIPRT_BITCODE)HIPRT_INCLUDE_DIR)find_package_handle_standard_args(HIPRTHIP_LINKER_EXECUTABLEis a hip component, checking on it here leads to the following bizarre log when it can't be found:Found HIPRT!.... excellent! .......Could NOT find HIPRT....wait..that's not what you just told me... you told me you found it..(missing: HIP_LINKER_EXECUTABLE)... hold up that's not even a hiprt component! that's a hip component!This check should be in
FindHIP.cmakewhich is the module that actually locatesHIP_LINKER_EXECUTABLEHIP_LINKER_EXECUTABLE which points to clang++ is a HIP component but if it is missing, it only affects enabling HIP-RT. When HIP-RT headers are present, Found HIPRT is appropriate, but if the HIP component required to compile the HIP-RT kernels is missing, then HIP-RT must be disabled. So, a more appropriate message instead of "Could NOT find HIPRT (missing: HIP_LINKER_EXECUTABLE)" would be something like "Could NOT find HIPRT dependency (missing: HIP_LINKER_EXECUTABLE)." Alternatively, if HIP is not found, then there is no need to look for HIP-RT.
That's a valid point, ideally the code around here could be changed to something like the following:
This naturally does mean,
findhip.cmakeneeds to be updated to deal with the optional components or we just accept the linker always will have to be found in which case we can skip the components part and just add a check it's set infindhip.cmakeI have a slight lean towards just always requiring the linker since its the least amount of work, unsure how @Sergey feels about this.
Originally, HIPCC_EXECUTABLE was required but not HIP_LINKER_EXECUTABLE. At some point, I noticed the linker was no longer found #121050 (comment) least the way my system was setup) and it led to kernel compilation failure during the build. So, I made the change to make the linker required for HIP-RT. If we can make sure that if HIPCC_EXECUTABLE is found then HIP_LINKER_EXECUTABLE is found, then always requiring the linker should be fine.
@LazyDodo I'm all in favor of not making CMake rules more complicated than they should be.
@LazyDodo and @Sergey
That's the simplest way I could come up with
9333515ce3Let me know if I need to make changes.
That's exactly what i initially envisioned, +1 from me
@LazyDodo @salipour Are there remaining points that needs addressing on Windows?
@ideasman42 Did you have a chance to have a look?
The .cpp files in the include folder still bother me, but given i also really want this diff to be finally off my plate, i'll take a commitment the hiprt team will fix this in a future update.
@LazyDodo the change to address this issue on hiprt side is done and pending review internally. It will eventually make it to the public repo.
Just to be sure, @Alaska you tested this on actual hardware on windows, correct?
Two weeks ago, I tested it on a RX 7800XT on Windows, but had to make some manual modifications to get it to work. Once it was working things seemed to be fine (no obvious regressions). Based on the commits I believe all the issues that required manual modification have been resolved.
I can retest it again with all the latest changes if you'd like.
I'd appreciate that very much!
All the build issues I encountered 2 weeks ago appear to be resolved and there were no new issues I could identify. Now it's as simple to setup and build as current main (ignoring the requirement to build the HIP-RT library).
Running the same tests I did previously (mainly the Cycles render tests), there's no obvious regressions, and the upgrading of the HIP-RT library as part of this PR improves some areas.
Needs a better commit message, but counting on @Sergey to take care of that while landing.
Rebuilding libs, will check on this soon.
Rebuilding libs, will test this PR when they're done.
Update: builds & tests pass (accepting).
@LazyDodo @ideasman42 Do you think it's possible to have libraries landed before this patch?
I don't think it will compile with the current state of libraries and wrangler. And if we can have libraries available in git-lfs already we can easily land this patch, without going into complications of either temporarily disabling HIP-RT, or trying to support both old/new style of libraries.
I've tweaked the description to explain a bit in details what it is about. Maybe @salipour would want to have another pass on it.
I was trying to get benchmark numbers, but it is a bit hard until #127464 is resolved (at least to have representative numbers). Don't think that this bug is a stopper for the this PR though.
two ways we could go about that
Personally i prefer 1
@LazyDodo 1/2 both seem OK.
To avoid delay from being in different time-zones, I'll commit the libs to a branch.
It might even be best to do this preemptively once the libs have been built & tested as it can avoid doing this all over again.
Pushed
temp-pr-121050to https://projects.blender.org/blender/lib-linux_x64/src/branch/temp-pr-121050@salipour Do you mind moving the change done to the dependency builder (
build_environmentfolder) to a separate PR, so we can land that part with the libraries?@Sergey here you go #127719
Let me know if you had something else in mind.
@salipour That is exactly what I had in mind.
For the next steps lets wait for the libraries to become available. I can help with making this PR pointing to a proper submodule then. Meanwhile maybe you can merge the
maininto this PR? Hopefully Git will be smart enough to recognize that build system part of it is the same, and will do the proper thing.