Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support CUDA 11 + linux fixes #23

Merged
merged 11 commits into from
Apr 17, 2023
Merged

Conversation

paulmelis
Copy link
Contributor

@paulmelis paulmelis commented Apr 16, 2023

This adds support for CUDA 11, but still checks for CUDA 12 in the main cmake config. The few places in the CUDA code where CUDA12-specific features are used are behind #include guards.

The patches in the patches/ dir need to be applied to the recursively checked out repository, as we need to patches to the glad, tiny-cuda-nn and pybind11 source repos in include/.

~~The first is needed as a shared library (and so the Python module) on Linux requires the code in the tiny-cuda-nn library to be built as position-independent code (-fPIC). I did not see the required options to enable this in the upstream code, hence the patches like this. ~~

The patch to pybind11 is needed when building against CUDA12 to work around pybind/pybind11#4606

Finally, I had an error related to the use of #pragma once, which I worked around using include guards.

//#pragma once
// error: #pragma once in main file
#ifndef TURBONERF_BIT_UTILS_CU
#define TURBONERF_BIT_UTILS_CU
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing this, i'm curious if removing this line from main.cu has the same effect?

#include "core/occupancy-grid.cuh"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried, it doesn't get rid of the error:

[ 36%] Building CUDA object CMakeFiles/PyTurboNeRF.dir/src/main.cu.o
/home/paulm/c/TurboNeRF/src/utils/bit-utils.cu:1:9: error: #pragma once in main file [-Werror]
    1 | #pragma once
      |         ^~~~
/home/paulm/c/TurboNeRF/src/utils/bit-utils.cu:1:9: warning: #pragma once in main file
    1 | #pragma once
      |         ^~~~

To be sure, here's what I changed for the test:

snellius paulm@gcn65 20:58 ~/c/TurboNeRF/build$ git diff
diff --git a/src/main.cu b/src/main.cu
index 6caa331..ab6dcdb 100644
--- a/src/main.cu
+++ b/src/main.cu
@@ -11,7 +11,7 @@
 #include "main.h"
 #include "controllers/nerf-training-controller.h"
 #include "controllers/nerf-rendering-controller.h"
-#include "core/occupancy-grid.cuh"
+//#include "core/occupancy-grid.cuh"
 #include "models/camera.cuh"
 #include "models/dataset.h"
 #include "models/render-request.cuh"
@@ -215,4 +215,4 @@ bool check_is_json_file(const std::string &path_str) {
                std::cout << fmt::format("Fileformat not JSON : {}", path_str);
                return false;
        }
-}
\ No newline at end of file
+}
diff --git a/src/utils/bit-utils.cu b/src/utils/bit-utils.cu
index 4108a79..29f41e1 100644
--- a/src/utils/bit-utils.cu
+++ b/src/utils/bit-utils.cu
@@ -1,7 +1,4 @@
-//#pragma once
-// error: #pragma once in main file
-#ifndef TURBONERF_BIT_UTILS_CU
-#define TURBONERF_BIT_UTILS_CU
+#pragma once
 
 #include "bit-utils.cuh"
 
@@ -25,4 +22,3 @@ __global__ void get_1s_per_uint32(
 
 TURBO_NAMESPACE_END
 
-#endif

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about also removing the include for occupancy-grid from nerf-training-controller.cuh?

(you'll need to add it to nerf-training-controller.cu the implementation file)

I just don't like that only one file uses header guards while every other file uses #pragma once

Copy link
Owner

@JamesPerlman JamesPerlman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @paulmelis are you ready for me to merge this?

@paulmelis
Copy link
Contributor Author

Yep, all done!

@paulmelis
Copy link
Contributor Author

Found a small error in the pybind11 patch while trying to apply it on a different workstation. Fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants