This is an archive of the discontinued LLVM Phabricator instance.

[DX][ObjYAML] Support for parsing DXIL part
ClosedPublic

Authored by beanz on Jun 1 2022, 10:24 AM.

Details

Summary

This patch adds support for parsing the DXIL part data into the
ObjectYAML tooling.

The DXIL part has additional headers describing the shader and bitcode
data and stores serialized bitcode after the headers.

Depends on D124945

Diff Detail

Event Timeline

beanz created this revision.Jun 1 2022, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 10:24 AM
beanz requested review of this revision.Jun 1 2022, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 10:24 AM
kuhar added inline comments.Jun 1 2022, 11:17 AM
llvm/include/llvm/BinaryFormat/DXContainer.h
100

In bytes or dwords?

101

I'm confused by this comment. Did you mean [BitcodeHeader.Size]? Also what is the content of this blob? I assume that it's not the bitcode, because it's starts after .Offset bytes, right?

110

nit: add an empty line between structs?

115

Maybe: // Number of uint32_t words including this header ?

125

It seems to me that this relies on the structs being tightly packed. But does it always have to be the case? I thought that the compiler is allowed to insert padding arbitrarily as long as all fields remain aligned.

In the serialization/deserialization code I worked on in the past, we always relied on the size of individual fields for this reason, and always read/wrote all structs field wise.

llvm/include/llvm/Object/DXContainer.h
118

This performs a copy, doesn't it? I guess that this may be desired because ProgramHeader is relatively small, right?

llvm/lib/ObjectYAML/DXContainerEmitter.cpp
105

nit: can you add an empty line in between functions?

121
153–154

Can we use OS.write_zeros(PadBytes) instead?

llvm/test/tools/obj2yaml/DXContainer/DXILPart.yaml
36–37

Is the whole blob necessary? Could we replace it with something shorter that is not a complete DXIL program but starts with the right magic only etc?

beanz added inline comments.Jun 1 2022, 11:51 AM
llvm/include/llvm/BinaryFormat/DXContainer.h
100

Bytes here, I'll be explicit in the comment.

101

Doh... Yes. The Offset is the offset from the start of the header.

The DXContainer format overly defines every offset and size.

125

In C struct members are "aligned in an implementation-defined manner appropriate to its type". The compiler can't arbitrarily add padding, but it can to ensure appropriate alignment. I explicitly added an Unused member to represent the padding that is expected, and that should make the remaining structure layout consistent with every architecture I'm aware of.

The assert is just intended to verify my bit math (and that none of our other architectures do anything weird) because code depends on it.

For binary file formats like object files we pretty regularly rely on structure definitions being C-standard layout. This is consistent throughout the BinaryFormat library.

llvm/include/llvm/Object/DXContainer.h
118

Yea, I don't love this. The ProgramHeader is small so the copy shouldn't hurt too much.

llvm/lib/ObjectYAML/DXContainerEmitter.cpp
121

I didn't want to do an early exit here because this will grow to cover more parts as I add support for them.

llvm/test/tools/obj2yaml/DXContainer/DXILPart.yaml
36–37

Yea, for the purposes of this test I can do a simplified case.

kuhar added inline comments.Jun 1 2022, 12:16 PM
llvm/include/llvm/BinaryFormat/DXContainer.h
125

I think that's fair. It's very unlikely for an implementation to over-align fields and a lot of other code would break too.

llvm/lib/Object/DXContainer.cpp
13

nit: cstddef? I think we generally prefer C++ versions of C headers

beanz marked 9 inline comments as done.Jun 6 2022, 1:26 PM
beanz updated this revision to Diff 434582.Jun 6 2022, 1:26 PM

Updates based on review feedback.

Thank you @kuhar!

kuhar accepted this revision.Jun 6 2022, 2:46 PM

LGTM

This revision is now accepted and ready to land.Jun 6 2022, 2:46 PM
This revision was landed with ongoing or failed builds.Jun 6 2022, 4:49 PM
This revision was automatically updated to reflect the committed changes.