Add support for VK_EXT_scalar_block_layout - #497
Conversation
00c73b4 to
86ead18
Compare
|
Edit: All sorted, as per @jeffbolznv's comment below. |
There was, essentially, a revlock between glslang/spirv-tools/spirv-headers for the recent change to rename NVX_raytracing->NV_ray_tracing. I think if you update all three to TOT these build issues will disappear. |
1141a45 to
46991f0
Compare
|
@jbolz I fixed the issues you pointed out, but still need to fix the tool dependency issues. It looks like @shannon-lunarg is now updating various repos to the 94 header, so I'll wait for that to go through before finishing this. |
0e26913 to
1e6a09c
Compare
|
@jeffbolznv yea that worked, was not obvious, but it worked. Find it odd how the dependencies of the shaderc repo are dictated by the android build in this repo, but sure... Anyway, everything should be fixed up and running as expected now. |
ec0cc14 to
95da128
Compare
|
Guh, right - just remembered that we added feature enables, so going to wait on that before declaring this "100% done and ready for review"... |
95da128 to
957b99d
Compare
Bit flags that don't have any members (e.g. VkRenderPassCreateFlagBits) don't generate in the vulkan headers, but were generating for the enum string helper - causing compile errors. Added a check for such types with no flags to fix the error.
Updated: - `build-android/known_good.json` - `scripts/known_good.json`
f15af31 to
ca84424
Compare
|
Ok now this is ready for final review. |
jzulauf-lunarg
left a comment
jzulauf-lunarg
left a comment
There was a problem hiding this comment.
Would like @chrisforbes to look at the spirv portion.
| "url" : "https://github.com/KhronosGroup/glslang.git", | ||
| "sub_dir" : "shaderc/third_party/glslang", | ||
| "commit" : "d509a5eec6f5a65b3ffdfa0a19096f338d2582be" | ||
| "commit" : "d2a7b07a64811bb4a734bd66ef4e0b4c7d7fe1af" |
There was a problem hiding this comment.
Why do we still have known good changes?
There was a problem hiding this comment.
The glslang version currently pointed to on master is still quite far back - it's on an old NVX_raytracing branch. Nothing else has had cause to update it since then, I guess?
There was a problem hiding this comment.
@shannon-lunarg @greg-lunarg -- glslang update look okay?
| : IsEnabled([=](const DeviceFeatures &features) { return features.transform_feedback_features.*ptr; }) {} | ||
| FeaturePointer(VkBool32 VkPhysicalDeviceFloat16Int8FeaturesKHR::*ptr) | ||
| : IsEnabled([=](const DeviceFeatures &features) { return features.float16_int8.*ptr; }) {} | ||
| FeaturePointer(VkBool32 VkPhysicalDeviceScalarBlockLayoutFeaturesEXT::*ptr) |
There was a problem hiding this comment.
@chrisforbes -- shader_validation piece look correct?
There was a problem hiding this comment.
I assume that since he's approved the review this is fine?
chrisforbes
left a comment
chrisforbes
left a comment
There was a problem hiding this comment.
Minor nit in test comments; otherwise LGTM.
Added code to use scalar block layout validation in spirv-val when VK_EXT_scalar_block_layout is enabled. Added positive test for this, mirroring the relaxed block layout extension test.
ca84424 to
142578a
Compare
|
@jzulauf-lunarg @jeffbolznv @shannon-lunarg @greg-lunarg It would be good to get this done before whatever the next SDK cycle is - I imagine a number of developers held off doing much with this prior to the new year, but people are going to start using this extension soon and will begin running into extraneous errors from the validation layers until this gets in. |
|
@Tobski the branch is reporting conflicts now. Can you please rebase off latest master so this can be landed? |
|
@jeffbolznv -- there's a glslang known good change due to land as well that we need to be compatible with @karl-lunarg . |
|
Yes, actually the raytracing test commit landed before mine and I had to resolve that. But I think that the known-goods have now been bumped to newer revisions than the ones being asked for here. So there is a good chance that simply dropping the known-good changes from this PR and rebasing will work. (But double-check) |
|
@Tobski -- I'll commit this early next week if you can push a version without the conflicts. |
|
@jzulauf-lunarg I'll sort this out tomorrow - thanks! |
|
@jzulauf-lunarg I've got a rebased version of the branch locally that simply removes the known good update commit, and it seems to work. I'm trying to push it, but for whatever reason it seems to be failing auth on my PC for some reason, not sure why... I'll keep prodding... |
|
Ok, GitHub desktop works for pushing for some reason, but that doesn't support force pushing, so just to unblock this - I've raised #567 - which has the correct state of the repo in it. Annoying to have to switch PRs but it should work... this is what I get trying to set up a new laptop :P |
|
Changes merged under #567 |
7 participants
Had to fix a couple things and update a bunch of known-goods, including in the Vulkan-Headers repo here: KhronosGroup/Vulkan-Headers#31 (which needs accepting before this goes in 😅). See commit log for details.Edit: No longer required, as @shannon-lunarg updated that repo separately to a later version.