Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IR: Add SPIR-V disassembly for embedded downstream IR dumps #6529

Merged
merged 7 commits into from
Mar 11, 2025
Merged

IR: Add SPIR-V disassembly for embedded downstream IR dumps #6529

merged 7 commits into from
Mar 11, 2025

Conversation

mkeshavaNV
Copy link
Contributor

When dumping IR that contains embedded downstream SPIR-V code (via EmbeddedDownstreamIR instructions), display the disassembled SPIR-V instead of just showing "". This makes the IR dumps more readable and useful for debugging SPIR-V code generation.

This CL also does:

  • Add SPIR-V tools integration to disassemble SPIR-V binary blobs
  • Add new precompiled-spirv-generics-test test to check for the disassembled SPIR-V

cheneym2
cheneym2 requested changes Mar 6, 2025
View reviewed changes
source/slang/CMakeLists.txt Outdated
@@ -230,6 +230,7 @@ set(slang_link_args
slang-reflect-headers
slang-lookup-tables
SPIRV-Headers
SPIRV-Tools
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slang-glslang already has spirv-tools support, no need to add this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source/slang/slang-ir.cpp Outdated
spv_diagnostic diagnostic = nullptr;

// Disassemble with friendly names and comments
spv_result_t result = spvBinaryToText(
Copy link
Collaborator

@cheneym2 cheneym2 Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use existing slang-glslang interface to spvtools, see https://github.com/shader-slang/slang/blob/master/source/slang/slang-emit.cpp#L2097

I think you can request the downstream compiler for "PassThroughMode::SpirvDis" instead, though it will return the same thing either way since the same downstream compiler supports Opt, Dis, Link, etc..

    IDownstreamCompiler* compiler = codeGenContext->getSession()->getOrLoadDownstreamCompiler(
        PassThroughMode::SpirvOpt,
        codeGenContext->getSink());
    if (compiler)
    {
#if 0
        // Dump the unoptimized/unlinked SPIRV after lowering from slang IR -> SPIRV
        compiler->disassemble((uint32_t*)spirv.getBuffer(), int(spirv.getCount() / 4));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

tests/library/export-library-generics-test.slang Outdated
// CHECK:let %126 : _ = interface_req_entry(%127, witness_table_t(%x5Fx5FBuiltinRealType))
// CHECK:let %128 : _ = interface_req_entry(%7, witness_table_t(%IFloat))
// CHECK:let %129 : _ = interface_req_entry(%130, Func(this_type(%x5Fx5FBuiltinFloatingPointType)))
// CHECK:[availableInDownstreamIR(6 : Int)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mkeshavaNV mkeshavaNV force-pushed the bugfix/gh-6513 branch 4 times, most recently from 7af2c0d to 37df7af Compare March 11, 2025 09:04
@mkeshavaNV mkeshavaNV added the pr: non-breaking PRs without breaking changes label Mar 11, 2025
@mkeshavaNV mkeshavaNV force-pushed the bugfix/gh-6513 branch 2 times, most recently from 87bb5b7 to d0ae2fb Compare March 11, 2025 10:02
@mkeshavaNV
Copy link
Contributor Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@slangbot slangbot mentioned this pull request Mar 11, 2025
Merged
@mkeshavaNV
IR: Add SPIR-V disassembly for embedded downstream IR dumps
d49f90d 
When dumping IR that contains embedded downstream SPIR-V code (via
EmbeddedDownstreamIR instructions), display the disassembled SPIR-V
instead of just showing "<binary blob>".

This CL also does:
- Adds a new interface for disassembly and get result.
- Modify export-library-generics.slang test test to check for the
  disassembled SPIR-V

Fixes shader-slang#6513
@mkeshavaNV
Copy link
Contributor Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

mkeshavaNV added a commit to mkeshavaNV/slang that referenced this pull request Mar 11, 2025
@mkeshavaNV
Merge pull request #8 from slangbot/format-6529-bugfix/shader-slanggh…
41ddf96 
…-6513

Format code for PR shader-slang#6529
@mkeshavaNV mkeshavaNV marked this pull request as ready for review March 11, 2025 10:27
@mkeshavaNV mkeshavaNV requested a review from a team as a code owner March 11, 2025 10:27
@aleino-nv aleino-nv requested a review from cheneym2 March 11, 2025 12:03
cheneym2
cheneym2 reviewed Mar 11, 2025
View reviewed changes
Copy link
Collaborator

@cheneym2 cheneym2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a couple suggestions

source/slang/slang-ir.cpp Outdated
// Special case EmbeddedDownstreamIR to show SPIR-V disassembly
if (op == kIROp_EmbeddedDownstreamIR)
{
auto targetInst = inst->getOperand(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put this logic in a helper function like dumpEmbeddedDownstream()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source/slang/slang-ir.cpp Outdated
}
else
{
dump(context, "<invalid SPIR-V>");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems misleading, we failed to get a disassembler, no fault of the SPIR-V itself. Maybe "unavailable disassembler" would be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source/slang/slang-ir.cpp Outdated
}
}
else
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a TODO comment for DXIL disassembly here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@mkeshavaNV
Add module-dual-target-verify test
64f3075 
Fixes shader-slang#6517
Adds a new test to verify that dxil and spirv targets are stored
separately in the precompiled blob.
@mkeshavaNV
Copy link
Contributor Author

@cheneym2 - I've updated PR with your review comments. I also pushed a commit for the dxil/spirv blob issue #6517
That issue and this one is quite related, so just added it as part of the same PR.

@mkeshavaNV
Copy link
Contributor Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@slangbot slangbot mentioned this pull request Mar 11, 2025
Merged
@mkeshavaNV mkeshavaNV enabled auto-merge (squash) March 11, 2025 13:42
@mkeshavaNV mkeshavaNV mentioned this pull request Mar 11, 2025
Closed
@mkeshavaNV mkeshavaNV merged commit f59e0ef into shader-slang:master Mar 11, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheneym2 cheneym2 cheneym2 approved these changes

Assignees
No one assigned
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkeshavaNV @slangbot @cheneym2