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.

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

DRAFT: Implement embedded precompiled dxil slang-module feature #4492

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cheneym2
Copy link
Collaborator

When specified "-embed-dxil" creates an implicit DXIL target which is compiled
with DXC and stored in the slang-module as an IR operation.

When a slang-module is loaded, any precompiled binaries are loaded too
and are used to skip IR linking and optimization stages, instead passing
through to DXC for linkage.

Add IR operations to embed precompiled DXIL or SPIR-V blobs
into IR. Adds a BlobLit literal that is mostly identical to
StringLit except for its inability to be displayed, e.g.
in dumped IR. In the future, the blob might be dumped as
hexadecimal, but for now it is summarized as "<binary blob>".
The options, '-embed-dxil' and '-embed-spirv' in slangc, cause
a target dxil or spirv to be compiled and stored in the
translation unit IR when written to a slang-module. Subsequent
changes intend to load the blobs from slang-modules when loaded
as to short-circuit additional processing and pass through
directly to DXC or Spirv-tools respectively, for linking.
Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

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

It looks good to me.
But I don't have any idea about the TU and TP problem.

@@ -706,6 +706,9 @@ extern "C"

/* When set, will generate SPIRV directly rather than via glslang. */
SLANG_TARGET_FLAG_GENERATE_SPIRV_DIRECTLY = 1 << 10,

/* When set, the output for this target will be embedded as extra data in IR. */
SLANG_TARGET_FLAG_EMBED_OUTPUT_IN_IR = 1 << 11,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to ignore this comment.
I think the name could reflect better of what it actually embeds if it is something like

SLANG_TARGET_FLAG_EMBED_DOWNSTREAM_COMPILER_BLOB_IN_IR

Comment on lines +2410 to +2411
dstSlice.chars = const_cast<char*>(inSlice.begin());
dstSlice.numChars = uint32_t(inSlice.getLength());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a question.
Is it necessary to create the temporary object, inSlice?
Can this be just like the following?

dstSlice.chars = (const char*)(blob->getBufferPointer());
dstSlice.numChars = blob->getBufferSize();

Comment on lines +1937 to +1938
case OptionKind::EmbedDXIL: SLANG_RETURN_ON_FAIL(addEmbeddedLibrary(CodeGenTarget::DXIL)); break;
case OptionKind::EmbedSPIRV: SLANG_RETURN_ON_FAIL(addEmbeddedLibrary(CodeGenTarget::SPIRV)); break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it will be easier to read if these are broken into multiple lines,

    case OptionKind::EmbedDXIL:
        SLANG_RETURN_ON_FAIL(addEmbeddedLibrary(CodeGenTarget::DXIL));
        break;
    case OptionKind::EmbedSPIRV:
        SLANG_RETURN_ON_FAIL(addEmbeddedLibrary(CodeGenTarget::SPIRV));
        break;

@@ -195,6 +195,14 @@ Result IRSerialWriter::write(IRModule* module, SerialSourceLocWriter* sourceLocW
switch (srcInst->getOp())
{
// Special handling for the ir const derived types
case kIROp_BlobLit:
{
// Blobs are serialized into string table like strings
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation here is off, because it uses tab characters.
We use space characters for indentations.

@jkwak-work jkwak-work added the pr: non-breaking PRs without breaking changes label Jun 27, 2024
@jkwak-work
Copy link
Collaborator

@cheneym2 , if you look at the right side, there is a clickable text, "Convert to draft".
It can make the PR actually a draft that wouldn't be submitted by a mistake.

@cheneym2 cheneym2 marked this pull request as draft June 27, 2024 18:56
@cheneym2 cheneym2 marked this pull request as draft June 27, 2024 18:56
@cheneym2
Copy link
Collaborator Author

@cheneym2 , if you look at the right side, there is a clickable text, "Convert to draft". It can make the PR actually a draft that wouldn't be submitted by a mistake.

@cheneym2 , if you look at the right side, there is a clickable text, "Convert to draft". It can make the PR actually a draft that wouldn't be submitted by a mistake.

Thanks, done. In gitlab, starting the merge request with DRAFT actually makes it unmergeable, I thought maybe github worked like that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants