-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove OpenGL support (part 1) #5626
Conversation
Fixes #5475 This removes the OpenGL backend (but *not* the OpenGLCompute backend) from public use: - Remove Target::OpenGL - remove DeviceAPI::GLSL - remove Func::glsl() and Func::shader() - remove all OpenGL-specific apps and tests - remove HalideRuntimeOpenGL.h - remove some internal code that is OpenGL-only Note that there is still internal code that needs trimming; since the OpenGLCompute backend uses some of the same code, and some of the same build deps, and some of the same runtime shared-library loading, I tried to err on the side of leaving code/buildrules/etc in place for now, with the plan to clean that up in subsequent PRs. Note also that feature Target::EGL is still present, as I believe it is still useful in conjunction with OpenGLCompute.
src/runtime/runtime_api.cpp
Outdated
(void *)&halide_opengl_run, | ||
(void *)&halide_opengl_wrap_render_target, | ||
(void *)&halide_opengl_wrap_texture, | ||
// (void *)&halide_opengl_context_lost, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why comment and not delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...scalpel left in patient, sorry, fixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, halide_opengl_create_context and halide_opengl_get_proc_address still appear to be needed by OpenGLCompute
Only failure is the rogue mul_div_mod failure:
|
README_cmake.md
Outdated
@@ -466,6 +464,8 @@ If the CMake version is lower than 3.18, the deprecated [`FindCUDA`][findcuda] | |||
module will be used instead. It reads the variable `CUDA_TOOLKIT_ROOT_DIR` | |||
instead of `CUDAToolkit_ROOT` above. | |||
|
|||
TODO: update this section for OpenGLCompute, which needs some (but maybe not all) of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you planning to get to this TODO before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I was, but since OpenGLCompute is ~untestable on desktop Linux until we fix the LLVM-exports issues (see #5627), I think it's probably more useful to leave these as TODO (but open an Issue to fix them once OpenGLCompute is testable again)...
src/StorageFlattening.cpp
Outdated
if (op->for_type == ForType::GPUBlock || | ||
op->for_type == ForType::GPUThread) { | ||
in_gpu = true; | ||
} | ||
// TODO: in_shader = true is no longer possible, clean up code accordingly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the branch that adds texture support for cuda might need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please point me at branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't appear to use in_shader
-- it adds a new clause to preserve the device-api
There are possibly some now unused intrinsics in IR.cpp: glsl_texture_* |
Done |
#5626 removed OpenGL support, but didn't remove this no-longer-needed class.
option(TARGET_OPENGL "Include OpenGL/GLSL target" ON) | ||
if (TARGET_OPENGL) | ||
target_compile_definitions(Halide PRIVATE WITH_OPENGL) | ||
endif () | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh! This breaks openglcompute! That macro is used later to enable OGLC stuff.
#if !defined(WITH_OPENGL) | ||
bad |= has_feature(Target::OpenGL) || has_feature(Target::OpenGLCompute); | ||
bad |= has_feature(Target::OpenGLCompute); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never defined via CMake after this PR.
Inadvertently removed code for properly enabling/disabling OGLC in #5626, this restores it properly
Inadvertently removed code for properly enabling/disabling OGLC in #5626, this restores it properly
Fixes #5475
This removes the OpenGL backend (but not the OpenGLCompute backend) from public use:
Note that there is still internal code that needs trimming; since the OpenGLCompute backend uses some of the same code, and some of the same build deps, and some of the same runtime shared-library loading, I tried to err on the side of leaving code/buildrules/etc in place for now, with the plan to clean that up in subsequent PRs.
Note also that feature Target::EGL is still present, as I believe it is still useful in conjunction with OpenGLCompute.