Updates for final Vulkan ray tracing extensions #2466

dgkoch · 2020-11-23T11:33:50Z

GLSL specs updates: KhronosGroup/GLSL#144
SPV spec updates: KhronosGroup/SPIRV-Registry#86

Depends on the following changes landing first:
SPIRV-Headers: KhronosGroup/SPIRV-Headers#182
SPIRV-Tools: KhronosGroup/SPIRV-Tools#4027

Update to final (non-provisional) SPIR-V capabilities (includes review feedback) - Change visibilty of findLinkerObjects. See merge request GLSL/glslang!78

dgkoch · 2020-11-23T11:53:40Z

cc @johnkslang

dneto0

Some failures are caused by using an older bison.
Using bison 3.7 will fix the "end of file" error messages

glslang/MachineIndependent/glslang_tab.cpp

Test/baseResults/rayQuery-global.rgen.out

dneto0 · 2020-11-23T18:16:17Z

Test/baseResults/spv.ext.ClosestHitShader_Subgroup.rchit.out

+26(incomingPayload):     25(ptr) Variable IncomingRayPayloadKHR
+              27:             TypePointer Input 10(int)
+28(gl_SubgroupInvocationID):     27(ptr) Variable Input
+              31:             TypeVector 10(int) 4


I'm getting a test difference around here:

- 31: TypeVector 10(int) 4 - 32: TypeInt 64 0 - 33: TypePointer Input 31(ivec4) + 31: TypeInt 64 0 + 32: TypeVector 10(int) 4 + 33: TypePointer Input 32(ivec4)

I haven't seen this issue...

I also have not seen this.

Maybe that's because we're not watching Travis anymore and not seeing multi-platform test differences?

Which bot tests are actually running all the tests and comparing results? Only Appveyor?

We lost Travis and added like 7 others instead, but I'm still unclear if they are all just build tests, whereas Travis tests 6 different execution environments.

dneto0 · 2020-11-23T18:29:34Z

I also recommend updating the known_good.json file in the root source directory, to ensure we validate against updated SPIRV-Tools.

johnkslang · 2020-11-23T18:35:07Z

I get a bunch of these "Validation failed" messages locally, when the only thing I change is spriv.hpp, and the corresponding spelling changes in glslang. It should make no difference at all in the generated result, and I verified that for at least one result, but the rebuilt validator (same code base, just the new header) says "Validation failed" on an extra 16 ray-tracing tests.

Edit: Update, that was regarding the previous appveyor diffs shown here. Now, it is down to just one of those, and a couple other things.

dgkoch · 2020-11-23T19:06:28Z

@johnkslang @dneto0 I think I've addressed all the outstanding issues in the latest push, including updated spirv-headers and spirv-tools.

johnkslang · 2020-11-23T19:12:59Z

I think this PR needs to move to use the latest spirv.hpp from SPIRV-Headers. (That's what I was doing locally, trying to make a PR to move to the latest header, but having a validator issue. In the meantime, discovered the header wouldn't match what's here anyway.)

E.g., see OpConvertUToAcclerationStructureKHR which is spelled more correctly in SPIRV-Headers as OpConvertUToAccelerationStructureKHR.

dneto0 · 2020-11-23T19:21:52Z

(Thumbs up to using latest spirv-headers and spirv-tools. I haven't looked at the spelling of the acceleration structure)

johnkslang · 2020-11-23T19:30:41Z

There are also differences in the switch statement for OpTerminateInvocation in spirv.hpp.

dgkoch · 2020-11-23T19:34:13Z

There are also differences in the switch statement for OpTerminateInvocation in spirv.hpp.

That seems to be a previous manual addition to spirv.hpp that seems to be missing in spriv-headers?

GLSL : https://gitlab.khronos.org/GLSL/GLSL/-/merge_requests/60 SPV : https://gitlab.khronos.org/spirv/spirv-extensions/-/merge_requests/182 See merge request GLSL/glslang!77

See merge request GLSL/glslang!81

Vulkan Issue KhronosGroup#2268

vulkan/vulkan#2230 - no layout specified should be same as std430 - explicitly test std140, std430, scalar layouts See merge request GLSL/glslang!86

vulkan/vulkan#2374 Add support for ignoreIntersectionEXT and terminateRayEXT as block terminator statements. See merge request GLSL/glslang!87

See merge request GLSL/glslang!88

And update mesh shader results

dneto0 · 2020-11-23T20:04:11Z

I think I found a bug resulting in indeterminate ordering of the output of the 64-bit int type vs. the ivec4 type.
It doesn't seem related to this PR?!
I'll file a separate PR for it.

dneto0 · 2020-11-23T20:06:34Z

Ah, this PR exposed the pre-existing bug with indeterminate order.

This is the fix:

$ git diff HEAD^
diff --git a/SPIRV/GlslangToSpv.cpp b/SPIRV/GlslangToSpv.cpp
index ba4264c2..a2494ec7 100644
--- a/SPIRV/GlslangToSpv.cpp
+++ b/SPIRV/GlslangToSpv.cpp
@@ -2122,8 +2122,9 @@ std::pair<spv::Id, spv::Id> TGlslangToSpvTraverser::getForcedType(glslang::TBuil
             // these require changing a 64-bit scaler -> a vector of 32-bit components
             if (glslangType.isVector())
                 break;
-            std::pair<spv::Id, spv::Id> ret(builder.makeVectorType(builder.makeUintType(32), 4),
-                                            builder.makeUintType(64));
+            spv::Id ivec4_type = builder.makeVectorType(builder.makeUintType(32), 4);
+            spv::Id uint64_type = builder.makeUintType(64);
+            std::pair<spv::Id, spv::Id> ret(ivec4_type, uint64_type);
             return ret;
         }
         // There are no SPIR-V builtins defined for these and map onto original non-transposed

The problem is that evaluation order of arguments is unspecified. It's compiler-dependent. So enforce an order.

@dgkoch ^

dgkoch · 2020-11-23T20:06:41Z

Updated to the latest spirv.hpp in this CL (early on).

johnkslang · 2020-11-23T20:08:44Z

So, #2468 passes the bot tests and has the same base known_good as this one.

Maybe it should go in first. (After looking into the non-determinism.)

dgkoch · 2020-11-23T20:10:56Z

So, #2468 passes the bot tests and has the same base known_good as this one.

Maybe it should go in first. (After looking into the non-determinism.)

I'd rather not put that one in. It'll conflict with all the test updates in this one.

dneto0 · 2020-11-23T20:13:45Z

(Sorry, I should have been clear. The indetrminate order issue is not a bug: generated code was always correct. But it interferes with maintaining checked-in tests. This PR was the first to exercise the issue.)

johnkslang · 2020-11-23T20:17:22Z

(Sorry, I should have been clear. The indetrminate order issue is not a bug: generated code was always correct. But it interferes with maintaining checked-in tests. This PR was the first to exercise the issue.)

But, historically (until a few months ago?), we would also have considered this an error, because we require getting the same test results on multiple compilers to pass the bot tests and turn green. I have fixed similar things before, due to that.

I'm worried this means we've dropped that kind of testing barrier in recent times, at least in part due to Travis CI not running.

johnkslang · 2020-11-23T20:20:33Z

I'd rather not put that one in. It'll conflict with all the test updates in this one.

If we put it in, this one would need a rebase. If we don't put it in, we need to manually sign off that we verified the header is identical to SPIRV-Headers (what I was doing that discovered some of these issues), which is probably acceptable.

For normal process in the future I highly recommend:

a PR that updates to the latest headers and tools, both the actual SPIRV/spirv.hpp and in the "known_good"
a follow-on PR that adds the new functionality, where the headers/tools are not changed

Authored-by: David Neto <dneto@google.com>

johnkslang · 2020-11-23T20:24:41Z

The non-determinism made appveyor get the other result.

Let's fix that as well, and see all the bots passing, before doing the merge. I think it's okay to do that as part of this PR; just one more small update to it.

dgkoch · 2020-11-23T20:35:28Z

Added David's patch from above.
The included spirv.hpp should exactly match what's in SPIRV-Headers.

Fix traceRay/executeCallable to have id instead of constant.

5917705

Update to final (non-provisional) SPIR-V capabilities (includes review feedback) - Change visibilty of findLinkerObjects. See merge request GLSL/glslang!78

dgkoch mentioned this pull request Nov 23, 2020

Task list for Vulkan Ray Tracing release KhronosGroup/Vulkan-Docs#1402

Closed

17 tasks

dneto0 mentioned this pull request Nov 23, 2020

Updates to final ray tracing extensions KhronosGroup/SPIRV-Tools#4027

Closed

dneto0 suggested changes Nov 23, 2020

View reviewed changes

glslang/MachineIndependent/glslang_tab.cpp Outdated Show resolved Hide resolved

glslang/MachineIndependent/glslang_tab.cpp Outdated Show resolved Hide resolved

dneto0 reviewed Nov 23, 2020

View reviewed changes

Test/baseResults/rayQuery-global.rgen.out Outdated Show resolved Hide resolved

dneto0 reviewed Nov 23, 2020

View reviewed changes

dgkoch force-pushed the khr_rt_final branch from 05c5006 to f9fbb72 Compare November 23, 2020 18:33

dgkoch force-pushed the khr_rt_final branch from f9fbb72 to f3301b1 Compare November 23, 2020 18:59

Add support for OpConvertUToAccelerationStructureKHR.

3d1566c

GLSL : https://gitlab.khronos.org/GLSL/GLSL/-/merge_requests/60 SPV : https://gitlab.khronos.org/spirv/spirv-extensions/-/merge_requests/182 See merge request GLSL/glslang!77

johnkslang mentioned this pull request Nov 23, 2020

Move to the most recent SPIRV-Headers and SPIRV-Tools. #2468

Closed

alelenv and others added 6 commits November 23, 2020 14:54

Add volatile qualifier to certain builtins for ray tracing.

921a71b

See merge request GLSL/glslang!81

make gl_RayTmaxEXT volatile in intersection shader

6b30406

Vulkan Issue KhronosGroup#2268

Add testing for layouts on SBT

4c61816

vulkan/vulkan#2230 - no layout specified should be same as std430 - explicitly test std140, std430, scalar layouts See merge request GLSL/glslang!86

Support for new opcodes OpIgnoreIntersectionKHR and OpTerminateRayKHR

f0cb0b1

vulkan/vulkan#2374 Add support for ignoreIntersectionEXT and terminateRayEXT as block terminator statements. See merge request GLSL/glslang!87

Fix code-generation issues with global ray query variables

a58ea4c

See merge request GLSL/glslang!88

update dependencies for spirv-headers and tools

b1a4eb1

And update mesh shader results

dgkoch force-pushed the khr_rt_final branch from f3301b1 to b1a4eb1 Compare November 23, 2020 20:03

Fix indeterminate argument ordering

526c3a2

Authored-by: David Neto <dneto@google.com>

johnkslang merged commit ffccefd into KhronosGroup:master Nov 23, 2020

dgkoch deleted the khr_rt_final branch November 23, 2020 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for final Vulkan ray tracing extensions #2466

Updates for final Vulkan ray tracing extensions #2466

dgkoch commented Nov 23, 2020 •

edited

Loading

dgkoch commented Nov 23, 2020

dneto0 left a comment

dneto0 Nov 23, 2020 •

edited

Loading

dgkoch Nov 23, 2020

johnkslang Nov 23, 2020

johnkslang Nov 23, 2020

dneto0 commented Nov 23, 2020

johnkslang commented Nov 23, 2020 •

edited

Loading

dgkoch commented Nov 23, 2020

johnkslang commented Nov 23, 2020

dneto0 commented Nov 23, 2020

johnkslang commented Nov 23, 2020

dgkoch commented Nov 23, 2020

dneto0 commented Nov 23, 2020

dneto0 commented Nov 23, 2020 •

edited

Loading

dgkoch commented Nov 23, 2020

johnkslang commented Nov 23, 2020

dgkoch commented Nov 23, 2020

dneto0 commented Nov 23, 2020

johnkslang commented Nov 23, 2020

johnkslang commented Nov 23, 2020 •

edited

Loading

johnkslang commented Nov 23, 2020

dgkoch commented Nov 23, 2020

Updates for final Vulkan ray tracing extensions #2466

Updates for final Vulkan ray tracing extensions #2466

Conversation

dgkoch commented Nov 23, 2020 • edited Loading

dgkoch commented Nov 23, 2020

dneto0 left a comment

Choose a reason for hiding this comment

dneto0 Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

dgkoch Nov 23, 2020

Choose a reason for hiding this comment

johnkslang Nov 23, 2020

Choose a reason for hiding this comment

johnkslang Nov 23, 2020

Choose a reason for hiding this comment

dneto0 commented Nov 23, 2020

johnkslang commented Nov 23, 2020 • edited Loading

dgkoch commented Nov 23, 2020

johnkslang commented Nov 23, 2020

dneto0 commented Nov 23, 2020

johnkslang commented Nov 23, 2020

dgkoch commented Nov 23, 2020

dneto0 commented Nov 23, 2020

dneto0 commented Nov 23, 2020 • edited Loading

dgkoch commented Nov 23, 2020

johnkslang commented Nov 23, 2020

dgkoch commented Nov 23, 2020

dneto0 commented Nov 23, 2020

johnkslang commented Nov 23, 2020

johnkslang commented Nov 23, 2020 • edited Loading

johnkslang commented Nov 23, 2020

dgkoch commented Nov 23, 2020

dgkoch commented Nov 23, 2020 •

edited

Loading

dneto0 Nov 23, 2020 •

edited

Loading

johnkslang commented Nov 23, 2020 •

edited

Loading

dneto0 commented Nov 23, 2020 •

edited

Loading

johnkslang commented Nov 23, 2020 •

edited

Loading