gpuav: Do GPU-AV Post Processing with uint32_t only #9390

spencer-lunarg · 2025-02-04T21:29:52Z

@arno-lunarg found #9381 was not good enough and we need to not use spirv::Instruction at all and resort to doing old-fashion vector<uint32_t> parsing

Added some helpers and I think it is tolerable (as parsing SPIR-V can be)

ci-tester-lunarg · 2025-02-04T21:29:56Z

CI Vulkan-ValidationLayers build queued with queue ID 364659.

ci-tester-lunarg · 2025-02-04T21:30:36Z

CI Vulkan-ValidationLayers build queued with queue ID 364675.

ci-tester-lunarg · 2025-02-04T21:35:02Z

CI Vulkan-ValidationLayers build # 18992 running.

ci-tester-lunarg · 2025-02-04T22:09:33Z

CI Vulkan-ValidationLayers build # 18992 failed.

ci-tester-lunarg · 2025-02-05T02:12:55Z

CI Vulkan-ValidationLayers build queued with queue ID 364950.

ci-tester-lunarg · 2025-02-05T02:15:02Z

CI Vulkan-ValidationLayers build # 18994 running.

ci-tester-lunarg · 2025-02-05T02:27:35Z

CI Vulkan-ValidationLayers build queued with queue ID 364968.

ci-tester-lunarg · 2025-02-05T02:30:01Z

CI Vulkan-ValidationLayers build # 18995 running.

ci-tester-lunarg · 2025-02-05T03:05:45Z

CI Vulkan-ValidationLayers build # 18995 passed.

ci-tester-lunarg · 2025-02-05T04:47:15Z

CI Vulkan-ValidationLayers build queued with queue ID 365055.

ci-tester-lunarg · 2025-02-05T04:50:01Z

CI Vulkan-ValidationLayers build # 18996 running.

layers/gpuav/instrumentation/gpuav_shader_instrumentor.cpp

ci-tester-lunarg · 2025-02-05T05:17:40Z

CI Vulkan-ValidationLayers build # 18996 passed.

arno-lunarg · 2025-02-05T07:31:07Z

layers/error_message/spirv_logging.cpp

-        const auto &insn = instructions[i];
-        if ((insn.Opcode() != spv::OpSource) || (insn.Length() < 5) || (insn.Word(3) != reported_file_id)) {
+    uint32_t offset = kModuleStartingOffset;
+    while (offset < instructions.size()) {


We can do better than this while loop, this old style is error prone: you have to think about updating length, updating offset every time you have a continue or at loop end, you have to update the correct length/offset, etc ...
Use a full fledged Instruction iterator class, that would benefit for this very cool Instruction class, and come up with something that does this for instance:

for (const Instruction& instruction : InstructionsSpan(instructions.data(), instructions.size()))

More work, but easier to use, way less error prone, and a good reason for you to explore C++ iterators further!

Or if you want to go fast with something not as elegant, at least go for that instead of the while loops:

for ( size_t offset = kModuleStartingOffset, length = Length(instruction[offset]); offset < instructions.size(); (offset += length, length = Length(instruction[offset])) )

I moved all the SPIR-V uint32_t parsing inside spirv_logging.cpp

I cleaned up the aweful nested while loops

Then I tried to do this iterator suggestions above (both the C++ and C style), I found adding it made things harder to read and while doing it, I had bug and it was not even obvious what was going on, so I reverted that part of the change

Every while (offset < instruction.size()) loop is 15 to 20 lines, with mainly a single offset += length and feel it's simplicity (and consistency) is good enough here

You might disagree, but I am satisfied with how the change is now and have no plan to update things, not "just" because this PR gets the job done, but because anyone who edits this can easily understand what is going on, it is the only file that does this, no one is going to honestly editing these isolated small functions (they been untouched forever since i added them). If this logic was actually doing more complex things (like the other parts of our SPIR-V code using the Instruction class, I would probably reconsider, but they literally just need to loop and pick out a single instruction, spirv-tool, spirv-reflect, spirv-cross, etc do the exact same thing here... its just the nature of parsing a IR

Then at least move to the for loop I described. It is just safer, and moves the iteration logic where it should be

I tried, it gets nasty for the double loops with SourceContinued as I need to still go

offset += length; // get next instructions break;

it is still the same problem, we just moved it to a more inconvenient location

Mmmmh you mean you can't go:

// first loop for ( size_t offset = kModuleStartingOffset, length = Length(instruction[offset]); offset < instructions.size(); (offset += length, length = Length(instruction[offset])) ) // second loop for ( ; offset < instructions.size(); (offset += length, length = Length(instruction[offset])) )

Looking at the code, I bet you 5$ you can

I feel this is harder to grasp, trying to shove all this stuff in the for loop header, but honestly sure, will just do it at this point because I need this PR in to move forward to add more Shader DebugInfo support

The while loop with manual increment is just copium, for loop where created to handle those kind of "oh but here I need to continue; so need to think about incrementing" problems

And looking through our code we definitely have existing bad while loop usage where a for loop would be better

This review keeps changing out from under me. But I like the while version better the for version because of the excessive comma operators required. KISS.

arno-lunarg

.

layers/error_message/spirv_logging.cpp

ci-tester-lunarg · 2025-02-06T04:42:07Z

CI Vulkan-ValidationLayers build queued with queue ID 365988.

ci-tester-lunarg · 2025-02-06T04:45:02Z

CI Vulkan-ValidationLayers build # 19005 running.

ci-tester-lunarg · 2025-02-06T05:12:16Z

CI Vulkan-ValidationLayers build # 19005 passed.

ci-tester-lunarg · 2025-02-06T05:21:52Z

CI Vulkan-ValidationLayers build queued with queue ID 366012.

ci-tester-lunarg · 2025-02-06T05:25:01Z

CI Vulkan-ValidationLayers build # 19006 running.

ci-tester-lunarg · 2025-02-06T05:51:33Z

CI Vulkan-ValidationLayers build # 19006 passed.

ci-tester-lunarg · 2025-02-06T14:50:50Z

CI Vulkan-ValidationLayers build queued with queue ID 366516.

spencer-lunarg · 2025-02-06T14:53:20Z

layers/error_message/spirv_logging.cpp

-            while (std::getline(in_stream, current_line)) {
-                out_source_lines.emplace_back(current_line);
-            }
+        next_offset = offset + length;  // point to next instruction after OpSource


ugh, now this is a subtle bug, we need to nest these for loops else we need to track the next instruction here

ci-tester-lunarg · 2025-02-06T14:55:03Z

CI Vulkan-ValidationLayers build # 19015 running.

ci-tester-lunarg · 2025-02-06T14:58:47Z

CI Vulkan-ValidationLayers build queued with queue ID 366547.

ci-tester-lunarg · 2025-02-06T15:00:01Z

CI Vulkan-ValidationLayers build # 19017 running.

ci-tester-lunarg · 2025-02-06T15:08:40Z

CI Vulkan-ValidationLayers build # 19006 passed.

jeremyg-lunarg

SPIRV parsing is such a PITA

jeremyg-lunarg · 2025-02-06T15:03:16Z

layers/error_message/spirv_logging.cpp

-        if ((insn.Opcode() != spv::OpSource) || (insn.Length() < 5) || (insn.Word(3) != reported_file_id)) {
+    uint32_t next_offset = 0;
+    for (uint32_t offset = kModuleStartingOffset, length = Length(instructions[offset]); offset < instructions.size();
+         (offset += length, length = Length(instructions[offset]))) {


it is a bit confusing on how the various length statements end up executing. Both here and on line 75. IMO it would be clearer if length was managed by standalone statements rather than torturing us with the comma operator

jeremyg-lunarg · 2025-02-06T15:05:21Z

layers/error_message/spirv_logging.cpp

-        if (insn.Length() < 7) {
-            return;  // Optional source Text not provided
+        // We have now found the proper OpExtInst DebugSource
+        out_file_string_id = instructions[offset + 5];


what do 4, 5 and 7 mean here? It might be helpful to comment something about how the instruction works so that it easier to see what is happening.

Nate always complained about this, basically it become a reality that if you are editing this code, you need the spec out for the IR instruciton

then you see that 5 here is <id> File

so I "could" make this instruction[offset + DebugSourceFileOperand] but this is constant issue we have everywhere and just never got around to code gen these operand names

jeremyg-lunarg · 2025-02-06T15:13:22Z

layers/error_message/spirv_logging.cpp

-        const auto &insn = instructions[i];
-        if ((insn.Opcode() != spv::OpSource) || (insn.Length() < 5) || (insn.Word(3) != reported_file_id)) {
+    uint32_t offset = kModuleStartingOffset;
+    while (offset < instructions.size()) {


This review keeps changing out from under me. But I like the while version better the for version because of the excessive comma operators required. KISS.

will re-review when this thing is rebased and settled down

spencer-lunarg requested a review from arno-lunarg February 4, 2025 21:29

spencer-lunarg requested a review from a team as a code owner February 4, 2025 21:29

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from 1aab813 to 168f29b Compare February 4, 2025 21:30

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from 168f29b to b82ce46 Compare February 5, 2025 02:12

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from b82ce46 to 1aff6e2 Compare February 5, 2025 02:27

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from 1aff6e2 to 86e7a43 Compare February 5, 2025 04:47

spencer-lunarg commented Feb 5, 2025

View reviewed changes

layers/gpuav/instrumentation/gpuav_shader_instrumentor.cpp Outdated Show resolved Hide resolved

arno-lunarg reviewed Feb 5, 2025

View reviewed changes

layers/error_message/spirv_logging.cpp Outdated Show resolved Hide resolved

arno-lunarg reviewed Feb 5, 2025

View reviewed changes

layers/error_message/spirv_logging.cpp Outdated Show resolved Hide resolved

arno-lunarg mentioned this pull request Feb 5, 2025

gpuav: Keep instruction count to make error message faster #9381

Closed

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from 86e7a43 to d943a71 Compare February 6, 2025 04:42

gpuav: Do GPU-AV Post Processing with uint32_t only

4eb750c

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from d943a71 to 4eb750c Compare February 6, 2025 05:21

spencer-lunarg commented Feb 6, 2025

View reviewed changes

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from 8aac0c2 to 3f2b707 Compare February 6, 2025 14:58

spencer-lunarg force-pushed the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch from 3f2b707 to 4eb750c Compare February 6, 2025 15:08

jeremyg-lunarg previously approved these changes Feb 6, 2025

View reviewed changes

jeremyg-lunarg self-requested a review February 6, 2025 15:23

arno-lunarg approved these changes Feb 6, 2025

View reviewed changes

spencer-lunarg merged commit b7d489b into KhronosGroup:main Feb 6, 2025
40 checks passed

spencer-lunarg deleted the spencer-lunarg-doing-spirv-bare-with-no-class-is-no-fun branch February 6, 2025 17:21

gpuav: Do GPU-AV Post Processing with uint32_t only #9390

gpuav: Do GPU-AV Post Processing with uint32_t only #9390

Conversation

spencer-lunarg commented Feb 4, 2025

ci-tester-lunarg commented Feb 4, 2025

ci-tester-lunarg commented Feb 4, 2025

ci-tester-lunarg commented Feb 4, 2025

ci-tester-lunarg commented Feb 4, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

ci-tester-lunarg commented Feb 5, 2025

arno-lunarg Feb 5, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencer-lunarg Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arno-lunarg left a comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

Choose a reason for hiding this comment

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

ci-tester-lunarg commented Feb 6, 2025

jeremyg-lunarg 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

arno-lunarg Feb 5, 2025 •

edited

Loading

spencer-lunarg Feb 6, 2025 •

edited

Loading