Glsl.meta.slang raytracing #3560 #10

jkwak-work · 2024-03-01T18:57:12Z

This PR is only for a code review purpose.

all functions part of ray_query, shader_invocation_reorder and ray_tracing_motion_blur are implemented for GLSL.meta. If HLSL lacks an implementation equal GLSL/SPIR-V was implemented without an HLSL equivalent in HLSL.meta (anticipation of HLSL equivalent function eventually); completed built in variables have only been stubbed thus far; NOT completed the following commit sets up formatting & tests for all ray_query functions (compute shader); completed the commit also sets up structure for compile testing glsl raytracing shaders; NOT completed

…L_EXT_ray_query, GLSL_NV_shader_invocation_reorder & GLSL_NV_ray_tracing_motion_blur Implement all functions & built-in variables (with tests) for the GLSL_EXT_ray_query, GLSL_NV_shader_invocation_reorder & GLSL_NV_ray_tracing_motion_blur glsl extension based on specification (https://github.com/KhronosGroup/GLSL/blob/main/extensions/nv/). implementation details of worth{ All functions with alternatives in GLSL->HLSL have been implemented in GLSL, HLSL & SPRI-V (hlsl implementation has a few errors); otherwise only GLSL & SPIR-V was implemented. * modified slang compiler to add support for query'ing and fetching raytracing payloads/attributes based on bound layout(location) and input a 'constexpr int'. This allows'dynamic typing' for these operations to late evaluate all location tied raytracing objects. An attribute to functions were also added to force setting a generic's type based on a payload object (this is also required to allow 'generic typed' raytracing object functions); all of the hacky generic forcing will be removed in next commit in regards to removing hlsl code GL_EXT_ray_tracing was NOT implemented and part of scope of this commit, they will be for the next commit This commit will be overrided with removing HLSL functions and logic. commit is happening just to keep code history } testing details of worth{ Raytracing shader (non compute, compute can test properly) tests only test compiling and opcodes due to testing system limitations }

…L_EXT_ray_tracing, GLSL_EXT_ray_query, GLSL_NV_shader_invocation_reorder & GLSL_NV_ray_tracing_motion_blur glsl extension based on specification (https://github.com/KhronosGroup/GLSL/blob/main/extensions/). implementation details of worth{ spirv required direct referencing of raytracing objects, as a result, __rayPayloadFromLocation, __rayAttributeFromLocation, and __rayCallableFromLocation, all take in a integer literal and use this to add a placeholder to fillin with an actual layout(location) bound object. layout(location) bound raytracing objects are never optimized out, this was intentionally added to the code. GL_EXT_ray_tracing was implemented as noted in the commit description. all HLSL support has been removed. } testing details of worth{ Raytracing shader tests only test compiling and opcodes due to testing system limitations (raygen, intersection, miss, anyhit cover all raytracing functions/builtins/syntax); Compute raytracing works with actual value testing }

change dictionary usage to be more correct syntax-wise compact and organize parser code to be more like other parser code (this structure is important since there will be many attributes along with layout() syntax later on) code style adjustments to match slang use of braces

jkwak-work

It looks good to me overall.

jkwak-work · 2024-03-01T19:16:19Z

tests/hlsl-intrinsic/shader-execution-reordering/hit-object-reorder-thread.slang

@@ -74,14 +76,14 @@ void rayGenerationMain()
        someValues);

    uint r = calcValue(hit);
-    // SPIRV: OpReorderThreadWithHitObjectNV


Why is this test removed?

I broke the tests during development; I forgot to add them back since I broke them at the start; I am adding them 👍

jkwak-work · 2024-03-01T19:17:31Z

tests/hlsl-intrinsic/shader-execution-reordering/hit-object-trace-ray.slang.expected

Why is this file removed?

these are not tests and go against the .gitignore; they don't do anything

jkwak-work · 2024-03-01T19:19:03Z

tests/glsl-intrinsic/raytracing/glsl-rayCompute.slang

+__generic<T : __BuiltinFloatingPointType, let N : int, let M : int>
+bool equals(matrix<T, N, M> lhs, matrix<T, N, M> rhs)
+{
+    for (int i = 0; i < N; i++)
+    {
+        for (int j = 0; j < M; j++)
+        {
+            if (
+                lhs[i][j] 
+                !=
+                rhs[i][j]
+                )
+            {
+                return false;
+            }
+        }
+    }
+    return true;
+}


Don't we have a equality testing function already for Matrix?

we do not, other testing code also has an "equals" function for this purpose

jkwak-work · 2024-03-01T19:31:11Z

source/slang/slang-ir.cpp

+            else if (kIROp_SPIRVAsmOperandRayCallableFromLocation == op)
+            {
+                varLayoutPointsTo = this->getRayCallableVariableFromLocation(intLitValue);
+            }
+            else
+            {
+                // should be impossible to hit
+                assert(false);
+            }


I think this can be better with

else { SLANG_ASSERT(kIROp_SPIRVAsmOperandRayCallableFromLocation == op); varLayoutPointsTo = this->getRayCallableVariableFromLocation(intLitValue); }

… impossible code

jkwak-work

I have some comments.

jkwak-work · 2024-03-04T18:56:15Z

source/slang/hlsl.meta.slang

+//__target_intrinsic(glsl, RayDesc)
+//__target_intrinsic(spirv, RayDesc)


Can you either uncomment these or remove?

jkwak-work · 2024-03-04T18:58:35Z

source/slang/hlsl.meta.slang

+    case _GL_EXT_ray_tracing:
+    case glsl: __intrinsic_asm "reportIntersectionEXT";


Is this change actually needed?
It seems like _GL_EXT_ray_tracing will cover all the GLSL target, doesn't it?

glsl->_GL_EXT_ray_tracing does/did not seem to work, this is needed
I think its a superset->subset type of problem (_GL_EXT_ray_tracing -> glsl is fine, other way is not)

Do you mean you don't need to have "case _GL_EXT_ray_tracing" when you have "case glsl"?

some code & tests enable/use the _GL_EXT_ray_tracing (hlsl->glsl) capability, I use glsl capability instead (but the code is identical since both are glsl)

jkwak-work · 2024-03-04T18:59:03Z

source/slang/hlsl.meta.slang

+    case glsl:
+    case glsl: 


This looks to be a mistake.
There should be only one "glsl" case.

jkwak-work · 2024-03-04T19:02:42Z

source/slang/hlsl.meta.slang

+            spirv_asm {
+                result:$$float = OpHitObjectGetCurrentTimeNV &this
+            };


Shouldn't this have "return"?

return spirv_asm { ... };

jkwak-work · 2024-03-04T19:09:44Z

source/slang/slang-emit-spirv.cpp

+            goto rayExtensionAddition;
        case kIROp_VulkanHitObjectAttributesDecoration:
+            // needed since GLSL will not set optypes accordingly, but will keep the decoration 
+            ensureExtensionDeclaration(UnownedStringSlice("SPV_NV_shader_invocation_reorder"));
+            requireSPIRVCapability(SpvCapabilityShaderInvocationReorderNV);
+            goto rayExtensionAddition;
        case kIROp_VulkanRayPayloadDecoration:
+        case kIROp_VulkanRayPayloadInDecoration:
+            // needed since GLSL will not set optypes accordingly, but will keep the decoration 
+            ensureExtensionDeclaration(UnownedStringSlice("SPV_KHR_ray_query"));
+            requireSPIRVCapability(SpvCapabilityRayQueryKHR);
+            goto rayExtensionAddition;
+rayExtensionAddition:;


Let's not use GOTO within switch statement.
In this case, you can set a temporary variable to be true and handle the action later.

bool isRayExtension = false; ... case kIROp_VulkanCallablePayloadInDecoration: ... isRayExtension = true; break; ... if (isRayExtension) { emitOpDecorateLocation(...); }

In general, "goto" should be limited to error handling; not for code reuse.

jkwak-work · 2024-03-04T19:18:00Z

source/slang/slang-ir.cpp

+    IRSPIRVAsmOperand* IRBuilder::emitSPIRVAsmOperandRayPayloadFromLocation(IRInst* inst)
+    {
+        SLANG_ASSERT(as<IRSPIRVAsm>(m_insertLoc.getParent()));
+        const auto i = createInst<IRSPIRVAsmOperand>(


It seems that you shouldn't have const here because your return type doesn't have const and "i" will be returned.

const is used since other functions which do the same thing also set a const ptr for return; likely const is used since in the function we are signaling that the ptr's value won't change value between instantiation and returning

"const" is not a "signal"-ing keyword, and it cannot automatically become non-const.
Something doesn't look right on this code to me.

I did some checking, in godbolt -- it should not compile in MSVC (without a flag), nor does it likely emit different ASM... I will check to see if there is a reason the other functions use a const~

nope, just a cause of warnings; I suppose I will remove all of the const'ness from these type of functions and if there is a question of why that will be a different discussion

jkwak-work · 2024-03-04T19:19:57Z

source/slang/slang-ir.h

@@ -2239,6 +2240,8 @@ struct IRAnalysis
    IRDominatorTree* getDominatorTree();
 };

+struct DiagnosticSink;


You may need to move this line up to the top where other classes are declared; around line 28~38.

jkwak-work · 2024-03-04T19:22:31Z

source/slang/slang-parser.cpp

@@ -7013,6 +7013,27 @@ namespace Slang
        {
            return SPIRVAsmOperand{ SPIRVAsmOperand::NonSemanticDebugPrintfExtSet, parser->ReadToken() };
        }
+        else if (AdvanceIf(parser, "__rayPayloadFromLocation")) {


You should keep the coding style same as how the previous code was written.
Please add a new line character between ")" and "{".
I see that this mistake occurs on multiple lines in this PR.

jkwak-work · 2024-03-04T19:25:43Z

source/slang/slang-parser.cpp

@@ -7771,6 +7793,20 @@ namespace Slang
            parser->ReadToken(TokenType::Comma);
        }

+#define CASE(key, type) if (AdvanceIf(parser, #key)) { auto modifier = parser->astBuilder->create<type>(); \
+    modifier->location = getIntegerLiteralValue(listBuilder.find<GLSLLayoutModifier>()->valToken); listBuilder.add(modifier); } else


Is "modifier->location" only for debugging purpose?

modifier->location is for creating a modifier and tying a glsl layout(location) location value to it

…aytracing_#3560

jkwak-work · 2024-03-05T14:09:10Z

Looks good to me

fix various code-style warnings fix vkray regression: remove assumption that layout-val will be attached to a vk object decoration (causes crashes in some specific senarios) add missing ext's to hlsl.meta removed __specialized_for_target(glsl) for specific function due to causing a regression and not doing anything functional

fixed lots of coding style issues added early intrinsic pass to handle spirv get_from_location and for the future 'glsl magic syntax'->'target with well defined syntax'

(once capabilities auto-add ext's, these problems will be no more!)

…ure)

added missing cap's to raytracing & adjusted syntax+naming to be conformant with Slang

…/ArielG-NV/slang into glsl.meta.slang_raytracing_#3560

…syntax + glsl syntax for raytracing objects as well, which is quite cool) changed comment about early raytracing object addition to ir (more about positioning of code emitted)

change code emit'ing to ensure it emits properly at top of scope refactored a bit of the descriptions to be more accurate for why we need things done the way they are

…adds to entry point)

…/ArielG-NV/slang into glsl.meta.slang_raytracing_#3560

…ess conflicts changed spirv replacement to be proper and not a hacky 'working' implementation (since the code is being finalized now and I don't need a 'just working' implementation for testing)

… no ref --> no HLSL export tag

ArielG-NV and others added 4 commits February 16, 2024 14:29

jkwak-work commented Mar 1, 2024

View reviewed changes

add back removed tests; adjust style as per requested with assert'ing…

aa994e0

… impossible code

jkwak-work commented Mar 4, 2024

View reviewed changes

ArielG-NV added 3 commits March 4, 2024 15:19

fixing code style & changing as per suggestions (9)

c9730f1

remove useless const's which just add warnings

d6b8ad9

Merge remote-tracking branch 'upstream/master' into glsl.meta.slang_r…

b0708bc

…aytracing_#3560

ArielG-NV force-pushed the glsl.meta.slang_raytracing_#3560 branch from 9f4ec6e to b8c3f4c Compare March 5, 2024 19:34

ArielG-NV and others added 17 commits March 5, 2024 14:48

fix warning->error with int cast

eb0c842

added capabilities to raytracing

410ca2b

fixed lots of coding style issues added early intrinsic pass to handle spirv get_from_location and for the future 'glsl magic syntax'->'target with well defined syntax'

add missing cap

3571474

checkin after fixing all tests(?)

5607641

fixed regressions by removing ext's when adding capabilities

c508fd1

(once capabilities auto-add ext's, these problems will be no more!)

remove wrong rayIntersection shader code (fails compile causing faili…

3be252c

…ure)

push vsproj files

fc1610c

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

34055a3

push vsproj files

507e9d5

adjust vs project filters

ea7f24e

realized I could just rerun the bat file...

b88070a

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

a603338

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

9031f30

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

bddc508

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

f9dcb96

adjusting syntax and code to conform with slang better

0f689bc

added missing cap's to raytracing & adjusted syntax+naming to be conformant with Slang

Merge branch 'glsl.meta.slang_raytracing_#3560' of https://github.com…

43dc4fb

…/ArielG-NV/slang into glsl.meta.slang_raytracing_#3560

ArielG-NV added 14 commits March 12, 2024 15:45

fixes to remove allowGLSL branch of code for raytracing (allows hlsl …

7802fcb

…syntax + glsl syntax for raytracing objects as well, which is quite cool) changed comment about early raytracing object addition to ir (more about positioning of code emitted)

add support for HLSL+GLSL raytracing (together)

a773525

change code emit'ing to ensure it emits properly at top of scope refactored a bit of the descriptions to be more accurate for why we need things done the way they are

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

788e19a

change how we simplify SPIRV so it works when we have no references (…

c4cfff6

…adds to entry point)

Merge branch 'glsl.meta.slang_raytracing_#3560' of https://github.com…

bd0efd9

…/ArielG-NV/slang into glsl.meta.slang_raytracing_#3560

fix warning

15e346b

remove unused loop

e486693

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

bcd9e91

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

0bc8fe9

fix failing matrix buffer pointer

d262b88

minimize all scope of changes such that merge into master will have l…

0ed454c

…ess conflicts changed spirv replacement to be proper and not a hacky 'working' implementation (since the code is being finalized now and I don't need a 'just working' implementation for testing)

fix forced emit'ing code hack due to finding source of problem being,…

7eaee63

… no ref --> no HLSL export tag

directly inline hanging call for clarity & styling

f25e104

Merge branch 'master' into glsl.meta.slang_raytracing_#3560

0a00950

jkwak-work closed this Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glsl.meta.slang raytracing #3560 #10

Glsl.meta.slang raytracing #3560 #10

jkwak-work commented Mar 1, 2024

jkwak-work left a comment

jkwak-work Mar 1, 2024

ArielG-NV Mar 1, 2024

jkwak-work Mar 1, 2024

ArielG-NV Mar 1, 2024

jkwak-work Mar 1, 2024

ArielG-NV Mar 1, 2024 •

edited

Loading

jkwak-work Mar 1, 2024

jkwak-work left a comment

jkwak-work Mar 4, 2024

jkwak-work Mar 4, 2024

ArielG-NV Mar 4, 2024 •

edited

Loading

jkwak-work Mar 5, 2024

ArielG-NV Mar 5, 2024 •

edited

Loading

jkwak-work Mar 4, 2024

jkwak-work Mar 4, 2024

jkwak-work Mar 4, 2024

jkwak-work Mar 4, 2024

ArielG-NV Mar 4, 2024 •

edited

Loading

jkwak-work Mar 5, 2024

ArielG-NV Mar 5, 2024 •

edited

Loading

ArielG-NV Mar 5, 2024 •

edited

Loading

jkwak-work Mar 4, 2024

jkwak-work Mar 4, 2024

jkwak-work Mar 4, 2024

ArielG-NV Mar 4, 2024 •

edited

Loading

jkwak-work commented Mar 5, 2024

		//__target_intrinsic(glsl, RayDesc)
		//__target_intrinsic(spirv, RayDesc)

		case _GL_EXT_ray_tracing:
		case glsl: __intrinsic_asm "reportIntersectionEXT";

Glsl.meta.slang raytracing #3560 #10

Glsl.meta.slang raytracing #3560 #10

Conversation

jkwak-work commented Mar 1, 2024

jkwak-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArielG-NV Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwak-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArielG-NV Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArielG-NV Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArielG-NV Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArielG-NV Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

ArielG-NV Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArielG-NV Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

jkwak-work commented Mar 5, 2024

ArielG-NV Mar 1, 2024 •

edited

Loading

ArielG-NV Mar 4, 2024 •

edited

Loading

ArielG-NV Mar 5, 2024 •

edited

Loading

ArielG-NV Mar 4, 2024 •

edited

Loading

ArielG-NV Mar 5, 2024 •

edited

Loading

ArielG-NV Mar 5, 2024 •

edited

Loading

ArielG-NV Mar 4, 2024 •

edited

Loading