WIP: Linux Support for HIP-RT #121050

Sahar A. Kashi · 2024-04-24T22:27:27+02:00

Sahar A. Kashi commented

2024-04-24 22:27:27 +02:00

HIP-RT was originally closed source and that created issue with distribution of HIP-RT runtime and kernel binaries. The changes add support for Linux but more importantly HIP-RT kernel binaries are generated during the build process, simplifying the distribution process, and unifying the compiler across all the components (BVH builder, traversal library and Cycles kernels).

In the current change list, the assumptions is that HIP-RT SDK is available under third party lib folder, and the folder structure is
top folder lib/hiprt
include files: hiprt/hiprt
dynamic library: hiprt/bin
dependency (headers only): hiprt/orochi

HIP-RT was originally closed source and that created issue with distribution of HIP-RT runtime and kernel binaries. The changes add support for Linux but more importantly HIP-RT kernel binaries are generated during the build process, simplifying the distribution process, and unifying the compiler across all the components (BVH builder, traversal library and Cycles kernels). In the current change list, the assumptions is that HIP-RT SDK is available under third party lib folder, and the folder structure is top folder lib/hiprt include files: hiprt/hiprt dynamic library: hiprt/bin dependency (headers only): hiprt/orochi

❤️ 3

Sahar A. Kashi added 4 commits 2024-04-24 22:27:39 +02:00

1e1a3aae07 1. Fix for Cycles Crash on deleting/hiding all mesh objects with HIP RT (#112983 )

2. Update for hip path to also include ROCm 6 driver

# Conflicts:
#	intern/cycles/device/hiprt/device_impl.cpp

6b47bb74a3 Switching to HIPRT 2.2

# Conflicts/Rebase main (3/26/24):
#	extern/hipew/src/hipew.c

3735e0b5f9 Submodule rebase update

cbe9cdb27a Adding Linux support, switching to HIP-RT open sourced

Sahar A. Kashi requested review from Brecht Van Lommel 2024-04-24 22:31:09 +02:00

Alaska commented

2024-04-25 03:41:08 +02:00

info_cfg_option(WITH_CYCLES_DEVICE_HIPRT) inside CmakeLists.txt should be inside the if (NOT APPLE) check above it.

`info_cfg_option(WITH_CYCLES_DEVICE_HIPRT)` inside `CmakeLists.txt` should be inside the `if (NOT APPLE)` check above it.

Shengyu Qu referenced this pull request

2024-04-25 18:15:48 +02:00

Cycles HIPRT device #104110

Brecht Van Lommel commented

2024-04-25 18:27:43 +02:00

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.

Brecht Van Lommel requested changes 2024-04-25 18:41:35 +02:00

Brecht Van Lommel left a comment

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.

CMakeLists.txt Outdated

						
				@ -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)

Brecht Van Lommel commented

2024-04-25 18:02:16 +02:00

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.

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.

build_files/build_environment/cmake/hiprt.cmake Outdated

						
				@ -0,0 +8,4 @@

				set(HIPRT_CMAKE_FLAGS ${DEFAULT_CMAKE_FLAGS})

				set(HIPRT_EXTRA_ARGS

				  --DHIPRT_EXPORTS=ON

Brecht Van Lommel commented

2024-04-25 18:00:50 +02:00

Should be -D instead of --D?

Should be `-D` instead of `--D`?

build_files/build_environment/cmake/hiprt.cmake

						
				@ -0,0 +39,4 @@

				)

				if(WIN32)

				  if(BUILD_MODE STREQUAL Release)

Brecht Van Lommel commented

2024-04-25 18:23:24 +02:00

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 "*")

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 "*") ```

build_files/build_environment/cmake/hiprt.cmake Outdated

						
				@ -0,0 +54,4 @@

				    ExternalProject_Add_Step(external_hiprt after_install

				      COMMAND ${CMAKE_COMMAND} -E copy

				        ${LIBDIR}/hiprt/dist/bin/Release/hiprt64.dll

				        ${HARVEST_TARGET}/hiprt/bin/hiprt64.dll

Brecht Van Lommel commented

2024-04-25 18:40:28 +02:00

I think hiprt64_d.dll should be used here.

I think `hiprt64_d.dll` should be used here.

build_files/build_environment/cmake/versions.cmake

						
				@ -856,2 +856,4 @@

				set(PYBIND11_HASH_TYPE MD5)

				set(PYBIND11_FILE pybind-v${PYBIND11_VERSION}.tar.gz)

				set(HIPRT_VERSION 2.3.7df94af)

Brecht Van Lommel commented

2024-04-25 18:28:09 +02:00

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.

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.

Ray molenkamp commented

2024-04-25 18:53:58 +02:00

URI is wrong, it's over at https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/releases/tag/2.3.7df94af

Brecht Van Lommel commented

2024-04-25 19:15:18 +02:00

Ah ok, so it's:

set(HIPRT_URI https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/archive/refs/tags/${HIPRT_VERSION}.zip)
set(HIPRT_HASH eeb4053fd7e5ada2e2dff838dff41ca1)

Ah ok, so it's: ``` set(HIPRT_URI https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/archive/refs/tags/${HIPRT_VERSION}.zip) set(HIPRT_HASH eeb4053fd7e5ada2e2dff838dff41ca1) ```

Ray molenkamp commented

2024-04-25 19:19:31 +02:00

probably? I haven't gotten around to building this yet i'll try to schedule some time for this over the weekend.

Brecht Van Lommel commented

2024-04-25 19:34:57 +02:00

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.

build_files/cmake/Modules/FindHIPRT.cmake

						
				@ -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)

Brecht Van Lommel commented

2024-04-25 18:37:47 +02:00

Is ${HIPRT_VERSION} supposed to be in the name? It's not in hiprt.cmake.

Is `${HIPRT_VERSION}` supposed to be in the name? It's not in `hiprt.cmake`.

Sahar A. Kashi commented

2024-05-02 00:53:08 +02:00

I updated the name in hiprt.cmake file.

build_files/cmake/Modules/FindHIPRT.cmake

						
				@ -53,0 +57,4 @@

				)

				if(HIPRT_LIB_DIR)

				 set(HIPRT_DYNAMIC_LIB_PATH

Brecht Van Lommel commented

2024-04-25 18:39:14 +02:00

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 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() ```

Sahar A. Kashi commented

2024-05-02 01:00:02 +02:00

The main intention was to check if the dynamic library for hiprt exists before enabling it.

intern/cycles/kernel/CMakeLists.txt

						
				@ -732,0 +797,4 @@

				    COMMAND ${hiprt_compile_command} ${hiprt_compile_flags_sdk_bc}

				    DEPENDS ${HIPRT_INCLUDE_DIR}/hiprt/impl/hiprt_kernels_bitcode.h)

Brecht Van Lommel commented

2024-04-25 18:24:27 +02:00

Nitpick: use 1 instead of 2 empty lines.

lib/windows_x64 Outdated

						
				@ -1 +1 @@

				Subproject commit a5521c85e03bfd1556ff1e63bf7163235c401497

				Subproject commit 19b2b87f5ef0d8caa39e0882fbf832052974b785

Brecht Van Lommel commented

2024-04-25 18:17:40 +02:00

This change to the submodule hash should be left out.

Sahar A. Kashi commented

2024-05-02 00:34:03 +02:00

Happened accidentally. How can I roll it back?

Brecht Van Lommel commented

2024-05-02 16:55:48 +02:00

Like this:

cd lib/windows_x64
git checkout a5521c85e03bfd1556ff1e63bf7163235c401497
cd ../..
git commit lib/windows_x64

Like this: ``` cd lib/windows_x64 git checkout a5521c85e03bfd1556ff1e63bf7163235c401497 cd ../.. git commit lib/windows_x64 ```

Sahar A. Kashi added 1 commit 2024-05-02 02:53:07 +02:00

d675edf94a HIPRT Linux, Code Review #1

This pull request is marked as a work in progress.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u HIPRT_OPEN_SOURCE:salipour-HIPRT_OPEN_SOURCE

git checkout salipour-HIPRT_OPEN_SOURCE

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Linux Support for HIP-RT #121050

Checkout