google / amber

Amber is a multi-API shader test framework
Apache License 2.0
193 stars 65 forks source link

Add support for VK_KHR_shader_subgroup_extended_types #998

Closed gfxstrand closed 1 year ago

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

dj2 commented 1 year ago

Code change LGTM, just waiting for the bots.

dj2 commented 1 year ago

I don't have permissions to ignore the bots, @dneto0 maybe does? They seem to have failed.

ben-clayton commented 1 year ago

I've kicked the bots manually (external contributions require the kokoro-run label).

CL-check-format failed with:

src/script.cc:132:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/vulkan/device.cc:502:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/vulkan/device.cc:504:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
samples/config_helper_vulkan.cc:808:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
samples/config_helper_vulkan.cc:872:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
samples/config_helper_vulkan.cc:1023:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
samples/config_helper_vulkan.cc:1091:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
samples/config_helper_vulkan.h:124:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

Looks like the change needs a clang-format.

dneto0 commented 1 year ago

Sorry, another two formatting problems:

samples/config_helper_vulkan.cc:808: Lines should be <= 80 characters long [whitespace/line_length] [2] samples/config_helper_vulkan.cc:1023: Lines should be <= 80 characters long [whitespace/line_length] [2]

gfxstrand commented 1 year ago

Those aren't fixable. VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_SUBGROUP_EXTENDED_TYPES_FEATURES_KHR is just too dang long. I guess I can drop the KHR suffix. That should fix one of them but the other one isn't going anywhere unless we remove a level of indentation from an entire function.

dj2 commented 1 year ago

If needed you can add // NOLINTNEXTLINE(whitespace/line_length) on the line before to disable the check.

dneto0 commented 1 year ago

The CI-ndk-build error is very unrelated. Failure to run the update build version info. I'll consider it non-blocking.

dneto0 commented 1 year ago

Ok,something is wonky with the SwiftShader build.

[1379/1515] Building CXX object third_party/swiftshader/src/Vulkan/CMakeFiles/vk_swiftshader.dir/Debug/EventListener.cpp.o FAILED: third_party/swiftshader/src/Vulkan/CMakeFiles/vk_swiftshader.dir/Debug/EventListener.cpp.o /bin/clang-10.0.0/bin/clang++ -D"DEBUGGER_WAIT_DIALOG" -DAMBER_CTS_VULKAN_HEADER=0 -DAMBER_ENABLE_CLSPV=0 -DAMBER_ENABLE_DXC=0 -DAMBER_ENABLE_LODEPNG=1 -DAMBER_ENABLE_RTTI=0 -DAMBER_ENABLE_SHADERC=1 -DAMBER_ENABLE_SPIRV_TOOLS=1 -DAMBER_ENABLE_VK_DEBUGGING=1 -DAMBER_ENGINE_DAWN=0 -DAMBER_ENGINE_VULKAN=1 -DENABLE_VK_DEBUGGER -DREACTOR_ENABLE_MEMORY_SANITIZER_INSTRUMENTATION -DSWIFTSHADER_ENABLE_ASTC -DVK_EXPORT="" -DVK_USE_PLATFORM_XCB_KHR -Dvk_swiftshader_EXPORTS -I../include -I../ -I../third_party/spirv-tools/include -I../third_party/swiftshader/src/Vulkan/.. -I../third_party/swiftshader/src/System/.. -I../third_party/swiftshader/third_party/marl/include -I../third_party/swiftshader/src/Pipeline/.. -I../third_party/swiftshader/include -I../third_party/spirv-headers/include -I../third_party/swiftshader/src/Pipeline -I../third_party/swiftshader/src/Device/.. -I../third_party/swiftshader/third_party/astc-encoder/Source -I../third_party/swiftshader/src/WSI/.. -I../third_party/swiftshader/src/Reactor/. -I../third_party/cppdap/include -m64 -fPIC -march=x86-64 -mtune=generic -g -g -g3 -fPIC -Wall -Wreorder -Wsign-compare -Wmissing-braces -Wextra -Wunreachable-code-loop-increment -Wunused-lambda-capture -Wstring-conversion -Wextra-semi -Wignored-qualifiers -Wdeprecated-copy -Wno-unneeded-internal-declaration -Wno-unused-private-field -Wno-comment -Wno-extra-semi -Wno-unused-parameter -Wno-unknown-warning-option -DENABLE_RR_PRINT -DSWIFTSHADER_LOGGING_LEVEL=Error -fno-exceptions -Wthread-safety -pthread -std=gnu++17 -MD -MT third_party/swiftshader/src/Vulkan/CMakeFiles/vk_swiftshader.dir/Debug/EventListener.cpp.o -MF third_party/swiftshader/src/Vulkan/CMakeFiles/vk_swiftshader.dir/Debug/EventListener.cpp.o.d -o third_party/swiftshader/src/Vulkan/CMakeFiles/vk_swiftshader.dir/Debug/EventListener.cpp.o -c ../third_party/swiftshader/src/Vulkan/Debug/EventListener.cpp In file included from ../third_party/swiftshader/src/Vulkan/Debug/EventListener.cpp:15: ../third_party/swiftshader/src/Vulkan/Debug/EventListener.hpp:60:42: error: no type named 'string' in namespace 'std' virtual void onSetBreakpoint(const std::string &func, bool &handled) {}



I'm looking.
dneto0 commented 1 year ago

I can reproduce the SwiftShader compile error using Clang 14. I wonder which part of the CI bit-rotted.

dneto0 commented 1 year ago

Filed https://github.com/google/amber/issues/999 for the swiftshader issue

dneto0 commented 1 year ago

Hey @gfxstrand. Thanks for your patience. Please rebase on top of latest main. We've removed the debugger support that had bit-rotted.

dneto0 commented 1 year ago

oh grand, I can rebase it myself!

dneto0 commented 1 year ago

Looks like the Android nkd-build flow breaks because it's using an older Android NDK. It seems to work fine with the latest NDK 25b.

dneto0 commented 1 year ago

Rebased to pick up the update to the Android NDK for the CI bots. This should work. (fingers crossed)

dneto0 commented 1 year ago

Filed #1003 for the failures on macos-clang-debug