Skip to content

[dxil2spv] Read file directly into llvm::MemoryBuffer#4304

Merged
sudonatalie merged 2 commits intomicrosoft:masterfrom
sudonatalie:file-handling
Mar 11, 2022
Merged

[dxil2spv] Read file directly into llvm::MemoryBuffer#4304
sudonatalie merged 2 commits intomicrosoft:masterfrom
sudonatalie:file-handling

Conversation

@sudonatalie
Copy link
Copy Markdown
Collaborator

Read DXIL file directly into llvm::MemoryBuffer rather than through an
IDxcBlobEncoding. This also allows support for supplying file by STDIN
(not yet with dedicated CLI).

Also clean up some CMake LLVM dependencies.

@sudonatalie sudonatalie requested a review from jaebaek March 1, 2022 21:12
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not versed on the output of llvm::MemoryBuffer::getFileOrSTDIN(..) but I am curious about the side effect of referencing the MemoryBuffer after calling release(). Is it ok in terms of the use-after-free?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There's no use-after-free risk, but moving the std::unique_ptr is preferable to releasing the raw pointer so I've switched it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If it has invalid DXIL program header, I think we have to print an error, don't we?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If the DXIL program header is invalid, this will fall through to emitError("Could not parse DXIL module")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is diagStr just used by hlsl::dxilutil::LoadModuleFromBitcode(..) or is it used to return a string?
If it somehow contains an error message, we have to print it and handle the error case.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There's actually just some recent unfinished cleanup here:

// Note: the DiagStr is not used.

I've propagated the note for now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we print the error message using err here (if module == nullptr)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How does LLVM_LINK_COMPONENTS work?
I guess somehow the llvm cmake build uses static/dynamic linking with the listed libraries.
You removed LLVM_LINK_COMPONENTS from tools/clang/tools/dxil2spv/CMakeLists.txt. I am curious how the listed libraries here linked with what target library.

Another concern is building it in Windows. Do we only target Linux build?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's just a helper variable that does the same llvm_map_components_to_libnames later on in CMake functions:

${LLVM_LINK_COMPONENTS}

Just a cleanup that follows the last PR where I introduced a clang library layer, since it's how the rest of the codebase's CMake files link LLVM components (both for windows and Linux builds).

Read DXIL file directly into llvm::MemoryBuffer rather than through an
IDxcBlobEncoding. This also allows support for supplying file by STDIN
(not yet with dedicated CLI).

Also clean up some CMake LLVM dependencies.
@jaebaek
Copy link
Copy Markdown
Collaborator

jaebaek commented Mar 10, 2022

LGTM!

@AppVeyorBot
Copy link
Copy Markdown

@sudonatalie sudonatalie merged commit aa26bfd into microsoft:master Mar 11, 2022
@sudonatalie sudonatalie deleted the file-handling branch March 11, 2022 00:53
sudonatalie added a commit to sudonatalie/DirectXShaderCompiler that referenced this pull request Jun 15, 2022
Revert "[dxil2spv] Print missing runtime errors (microsoft#4481)"

This reverts commit 7f8f9ac.

Revert "[dxil2spv] Command line parsing and output file (microsoft#4454)"

This reverts commit 2e49e68.

Revert "[dxil2spv] Add additional error checking (microsoft#4440)"

This reverts commit 3c1918e.

Revert "[dxil2spv] Add FileCheck testing (microsoft#4431)"

This reverts commit d6cec44.

Revert "[dxil2spv] Translate DXIL constants to SPIR-V (microsoft#4426)"

This reverts commit f00b3c2.

Revert "[dxil2spv] Translate SV_Position to BuiltIn::Position (microsoft#4423)"

This reverts commit b292f94.

Revert "[dxil2spv] Translate extractValue and bufferStore (microsoft#4404)"

This reverts commit 1d2a68f.

Revert "[dxil2spv] Translate createHandle and bufferLoad (microsoft#4389)"

This reverts commit 316b849.

Revert "[dxil2spv] Support shl instruction (microsoft#4388)"

This reverts commit 1039a87.

Revert "[dxil2spv] Fix build warning (microsoft#4371)"

This reverts commit 7ecebb3.

Revert "[dxil2spv] Add support for dx.op.threadId (microsoft#4354)"

This reverts commit c248a1e.

Revert "[dxil2spv] Add initial compute shader support (microsoft#4345)"

This reverts commit c701ece.

Revert "[dxil2spv] Add error checking to file tests (microsoft#4344)"

This reverts commit 9f70135.

Revert "[dxil2spv] Add passthrough vertex shader and test (microsoft#4337)"

This reverts commit c99f5a5.

Revert "[dxil2spv] SPIR-V validation and related fixes (microsoft#4328)"

This reverts commit 4958718.

Revert "[dxil2spv] Implement simple entry function creation (microsoft#4323)"

This reverts commit c29a0c7.

Revert "[dxil2spv] Read file directly into llvm::MemoryBuffer (microsoft#4304)"

This reverts commit aa26bfd.

Revert "[dxil2spv] Add stage IO variables (microsoft#4271)"

This reverts commit 2d5f186.

Revert "[dxil2spv] Add CompilerInstance object (microsoft#4229)"

This reverts commit e4cf486.

Revert "[dxil2spv] Add initial testing (microsoft#4222)"

This reverts commit c3a61b1.

Revert "[dxil2spv] Construct minimal SPIR-V module (microsoft#4216)"

This reverts commit e3da12e.

Revert "[dxil2spv] Add basic dxil2spv CI test (microsoft#4209)"

This reverts commit 57d4d2b.

Revert "Add initial dxil2spv executable (microsoft#4199)"

This reverts commit c45db48.
sudonatalie added a commit that referenced this pull request Jun 16, 2022
Revert "[dxil2spv] Print missing runtime errors (#4481)"

This reverts commit 7f8f9ac.

Revert "[dxil2spv] Command line parsing and output file (#4454)"

This reverts commit 2e49e68.

Revert "[dxil2spv] Add additional error checking (#4440)"

This reverts commit 3c1918e.

Revert "[dxil2spv] Add FileCheck testing (#4431)"

This reverts commit d6cec44.

Revert "[dxil2spv] Translate DXIL constants to SPIR-V (#4426)"

This reverts commit f00b3c2.

Revert "[dxil2spv] Translate SV_Position to BuiltIn::Position (#4423)"

This reverts commit b292f94.

Revert "[dxil2spv] Translate extractValue and bufferStore (#4404)"

This reverts commit 1d2a68f.

Revert "[dxil2spv] Translate createHandle and bufferLoad (#4389)"

This reverts commit 316b849.

Revert "[dxil2spv] Support shl instruction (#4388)"

This reverts commit 1039a87.

Revert "[dxil2spv] Fix build warning (#4371)"

This reverts commit 7ecebb3.

Revert "[dxil2spv] Add support for dx.op.threadId (#4354)"

This reverts commit c248a1e.

Revert "[dxil2spv] Add initial compute shader support (#4345)"

This reverts commit c701ece.

Revert "[dxil2spv] Add error checking to file tests (#4344)"

This reverts commit 9f70135.

Revert "[dxil2spv] Add passthrough vertex shader and test (#4337)"

This reverts commit c99f5a5.

Revert "[dxil2spv] SPIR-V validation and related fixes (#4328)"

This reverts commit 4958718.

Revert "[dxil2spv] Implement simple entry function creation (#4323)"

This reverts commit c29a0c7.

Revert "[dxil2spv] Read file directly into llvm::MemoryBuffer (#4304)"

This reverts commit aa26bfd.

Revert "[dxil2spv] Add stage IO variables (#4271)"

This reverts commit 2d5f186.

Revert "[dxil2spv] Add CompilerInstance object (#4229)"

This reverts commit e4cf486.

Revert "[dxil2spv] Add initial testing (#4222)"

This reverts commit c3a61b1.

Revert "[dxil2spv] Construct minimal SPIR-V module (#4216)"

This reverts commit e3da12e.

Revert "[dxil2spv] Add basic dxil2spv CI test (#4209)"

This reverts commit 57d4d2b.

Revert "Add initial dxil2spv executable (#4199)"

This reverts commit c45db48.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants