Fix issues in OpenGL backend #5545
Conversation
OpenGL relies on the correct order of kernel args, so we can only sort the args on non-OpenGL target.
See also this commit: halide#5297
OpenGL runtime will use raw type pointer to retrieve the uniform value in kernel args, so we have to use C-style type definition instead of GLSL-style. See also halide#4702
GL 2.x does not support uint/uvec and only a subset of builtin functions support int/ivec arguments. Besides, GLES does not define round of integer division.
VAO (vertex array object) must be bind before vertex attribute array is enabled or disabled.
Bypass integer division temporarily since the latest version is far more complicated to list.
|
Now all 20 OpenGL tests passed. Tested under Mac and Linux. |
|
LGTM pending green tests at #5550 |
|
Will this fix #4937 (the glsl app)? https://github.com/halide/Halide/tree/master/apps/glsl |
Tested under linux, and it (apps/glsl) looks good. |
That's great! Would you please enable the app in apps/CMakeLists.txt? |
|
Waiting for the CMake update for apps/glsl; once that's in place, will re-run tests, and land if all good |
Sorry for my late... |
|
Note that apps/opengl_test is failing because it apparently requires X11 to be functional, which isn't the case on our buildbots |
Do you mean apps/opengl_demo? or apps/glsl? |
src/CodeGen_OpenGL_Dev.cpp
Outdated
| if (type.bits() == 32) { | ||
| result = Int(32); | ||
| } else { | ||
| result = Float(32); | ||
| } |
There was a problem hiding this comment.
I'm trying to understand this case. Why is it valid to compile a uint to a signed int or float? Should there be a warning here that the users' requested type isn't being honored?
There was a problem hiding this comment.
apps/opengl_test
Do you mean apps/opengl_demo? or apps/glsl?
https://buildbot.halide-lang.org/master/#/builders/25/builds/6
There was a problem hiding this comment.
@alexreinking Basically these code around support_native_uint exists before commit #60442cf9
Sorry I forgot the comments and have add those back. The idea about represent uint by int seems a compromise and may result in overflows and undefined behavior
It may be fixed to ignore the X11 library in MacOS and try to use the EGL feature in Linux to avoid the "Could not open X11 display" error in Buildbot headless environment. |
Basically these code and comments around `support_native_uint` and `support_non_float_type_builtin` exist before commit #60442cf9
cmake/HalideGeneratorHelpers.cmake
Outdated
| if (NOT "${ARGN}" MATCHES "egl" AND NOT TARGET X11::X11) | ||
| find_package(X11) | ||
| if (NOT X11_FOUND) | ||
| message(AUTHOR_WARNING "X11 dependency not found on system.") | ||
| endif () | ||
| endif () | ||
| target_link_libraries(${TARGET} ${VISIBILITY} X11::X11) | ||
| if (X11_FOUND) | ||
| target_link_libraries(${TARGET} ${VISIBILITY} X11::X11) | ||
| endif () |
There was a problem hiding this comment.
This is wrong. X11_FOUND is set as a cache variable so X11 will be linked to every target after the first one loads X11 (or if it is loaded previously). I think this is what you meant (it seems you intended to link X11 only if "egl" is not in the target)
if (NOT "${ARGN}" MATCHES "egl")
if (NOT TARGET X11::X11)
find_package(X11 REQUIRED)
if (NOT X11_FOUND)
message(AUTHOR_WARNING "X11 dependency not found on system.")
endif ()
endif ()
target_link_libraries(${TARGET} ${VISIBILITY} X11::X11)
endif ()
|
@alexreinking Thanks! |
|
@alexreinking I remembered that MacOS did not have EGL but didn't need X11 linking either. Should I add |
Nah, if a user requests EGL on macOS it should blow up. If you want, you can use that check to produce an AUTHOR_WARNING. |
Not that issue. I mean, on macOS it does not use EGL feature and the generator will fail to link X11 library. Should we add runtime checking around the X11 in CMake? |
|
Looks like this is still failing on OSX with "X11 not found", see https://buildbot.halide-lang.org/master/#/builders/33/builds/72 |
Yeah, then go ahead and add "if linux" around that dependency. I guess I'm not totally clear when libX11 is needed? |
|
Add "if linux" around the X11 dependency. |
|
@alexreinking It seems that Linux will use |
|
@xndcn - Turns out that |
|
Okay there's a bunch of stuff I learned by trying to run this locally.
|
|
@xndcn - I reworked the CMake stuff a bit and pushed to your branch, just so you know. |
|
@xndcn - I added code to load |
|
Build finally green, landing |
|
@xndcn - we're having issues on our newer buildbots where EGL is getting the wrong default display. Basically, it's picking the GBM platform instead of the NVidia device platform. It's looking for a pbuffer-enabled surface, but only "win" is available through GBM. Any ideas how we can fix this? It grabs the wrong display here: Halide/src/runtime/opengl_egl_context.cpp Line 75 in 734fec8 And fails to find the configuration here: Halide/src/runtime/opengl_egl_context.cpp Lines 120 to 144 in 734fec8 Here's the |
Fix some issues in OpenGL backend.
Fixes #4885
Fixes #4937