This is an archive of the discontinued LLVM Phabricator instance.

[DirectX] Add DirectX target object writer
ClosedPublic

Authored by beanz on Jun 6 2022, 4:32 PM.

Details

Summary

This is the last piece to bring together writing DXContainer files
containing DXIL through the DirectX backend.

While this change only has one test, all of the tests under
llvm/test/tools/dxil-dis also exercise this code. With this change the
output object file type for the dxil target is now DXContainer. Each of
the existing tests will generate DXContainer files, and the dxil-dis
tests additionally verify that the DXContainers generated are
well-formed and can be parsed by the DirectXShaderCompiler tools.

Depends on D127153 and D127165

Diff Detail

Event Timeline

beanz created this revision.Jun 6 2022, 4:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 4:32 PM
beanz requested review of this revision.Jun 6 2022, 4:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 4:32 PM
beanz updated this revision to Diff 434652.Jun 6 2022, 5:31 PM

Updating test case to verify shader version and kind fields.

beanz updated this revision to Diff 434656.Jun 6 2022, 5:48 PM

Fix a typo

kuhar added inline comments.Jun 6 2022, 8:32 PM
llvm/lib/Target/DirectX/DirectXSubtarget.h
47
llvm/lib/Target/DirectX/MCTargetDesc/DirectXMCTargetDesc.cpp
138–139

nit: return directly? Also below.

beanz marked 2 inline comments as done.Jun 7 2022, 9:28 AM
beanz updated this revision to Diff 434847.Jun 7 2022, 9:29 AM

Updates based on review feedback. Thanks @kuhar!

kuhar accepted this revision.Jun 7 2022, 10:46 AM

LGTM but please get a second approval before submitting

This revision is now accepted and ready to land.Jun 7 2022, 10:46 AM
MaskRay added inline comments.Jun 10 2022, 2:49 PM
llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
107
llvm/lib/Target/DirectX/MCTargetDesc/DirectXContainerObjectWriter.cpp
21

delete blank line

llvm/test/CodeGen/DirectX/embed-dxil.ll
25

[[#]] in place of {{[0-9]+}}

beanz updated this revision to Diff 437920.Jun 17 2022, 8:34 AM

Updates based on feedback from @MaskRay! Thank you!

python3kgae added inline comments.Jun 17 2022, 8:59 AM
llvm/test/CodeGen/DirectX/embed-dxil.ll
4

Is it possible to extract the dxil part out of the container and run dxil-dis to test the output dxil?

python3kgae added inline comments.Jun 17 2022, 9:02 AM
llvm/test/CodeGen/DirectX/embed-dxil.ll
5

better use library instead of compute.
This is not a legal compute shader.

beanz added inline comments.Jun 17 2022, 10:13 AM
llvm/test/CodeGen/DirectX/embed-dxil.ll
4

There are a few reasons _not_ to use dxil-dis here.

First and foremost, this test is testing the structure of the generated DXContainer file, not the dxil encoding itself. In that regard dxil-dis doesn't help because while it can extract the bitcode from a DXContainer, it doesn't actually dump any of the structural information about the container itself (which is what is important here).

Additionally dxil-dis is an optional external dependency. Tests that rely on dxil-dis only run if you have dxil-dis available, and I don't expect most LLVM developers will have dxil-dis available. Maximizing test coverage while not depending on external dependencies is a big win.

beanz updated this revision to Diff 437958.Jun 17 2022, 10:15 AM

Changing shader to library. Thanks @python3kgae!

beanz added inline comments.Jun 17 2022, 10:51 AM
llvm/test/CodeGen/DirectX/embed-dxil.ll
4

Also worth noting here: with this change llc with -filetype=obj will produce DXContainer files when targeting DXIL. This will result in all the existing dxil-dis tests that use llc to operate on DXContainers instead of bitcode files. The tests all work as long as you have a reasonably up-to-date dxil-dis because that feature was added to dxil-dis recently.

python3kgae added inline comments.Jun 17 2022, 10:53 AM
llvm/test/CodeGen/DirectX/embed-dxil.ll
4

Then could we have a separate dxil-dis test in dxil-dis folder to make sure the whole pipeline works?

beanz added inline comments.Jun 17 2022, 11:09 AM
llvm/test/CodeGen/DirectX/embed-dxil.ll
4

We don't need to add tests for that. The existing dxil-dis tests will already provide that coverage.

python3kgae accepted this revision.Jun 17 2022, 11:28 AM
This revision was landed with ongoing or failed builds.Jun 17 2022, 7:33 PM
This revision was automatically updated to reflect the committed changes.