[PATCH] D126795: [DX][ObjYAML] Support for parsing DXIL part

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:100
+  uint32_t Offset; // Offset to LLVM bitcode (from start of header).
+  uint32_t Size;   // Size of LLVM bitcode.
+  // Followed by uint8_t[BitcodeHeader.BitcodeOffset]
----------------
kuhar wrote:
> In bytes or dwords?
Bytes here, I'll be explicit in the comment.


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:101
+  uint32_t Size;   // Size of LLVM bitcode.
+  // Followed by uint8_t[BitcodeHeader.BitcodeOffset]
+
----------------
kuhar wrote:
> 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?
Doh... Yes. The `Offset` is the offset from the start of the header.

The DXContainer format overly defines every offset and size.


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:125
+
+static_assert(sizeof(ProgramHeader) == 24, "ProgramHeader Size incorrect!");
+
----------------
kuhar wrote:
> 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.
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.


================
Comment at: llvm/include/llvm/Object/DXContainer.h:118
+
+  Optional<DXILData> getDXIL() const { return DXIL; }
 };
----------------
kuhar wrote:
> This performs a copy, doesn't it? I guess that this may be desired because `ProgramHeader` is relatively small, right?
Yea, I don't love this. The ProgramHeader is small so the copy shouldn't hurt too much.


================
Comment at: llvm/lib/ObjectYAML/DXContainerEmitter.cpp:121
 
-    // TODO: Write Part data
+    if (P.Name == "DXIL" && P.Program) {
+      dxbc::ProgramHeader Header;
----------------
kuhar wrote:
> Consider an early exit instead: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
I didn't want to do an early exit here because this will grow to cover more parts as I add support for them.


================
Comment at: llvm/test/tools/obj2yaml/DXContainer/DXILPart.yaml:36-37
+      DXILSize:        1556
+      DXIL:            [ 0x42, 0x43, 0xC0, 0xDE, 0x21, 0xC, 0x0, 0x0, 0x82, 
+                         0x1, 0x0, 0x0, 0xB, 0x82, 0x20, 0x0, 0x2, 0x0, 
+                         0x0, 0x0, 0x13, 0x0, 0x0, 0x0, 0x7, 0x81, 0x23, 
----------------
kuhar wrote:
> 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?
Yea, for the purposes of this test I can do a simplified case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126795/new/

https://reviews.llvm.org/D126795