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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar 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]
----------------
In bytes or dwords?


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:101
+  uint32_t Size;   // Size of LLVM bitcode.
+  // Followed by uint8_t[BitcodeHeader.BitcodeOffset]
+
----------------
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?


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:110
+};
+struct ProgramHeader {
+  uint8_t MinorVersion : 4;
----------------
nit: add an empty line between structs?


================
Comment at: llvm/include/llvm/BinaryFormat/DXContainer.h:115
+  uint16_t ShaderKind;
+  uint32_t Size; // Size in uint32_t units including this header.
+  BitcodeHeader Bitcode;
----------------
Maybe: `// Number of uint32_t words including this header `?


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


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


================
Comment at: llvm/lib/ObjectYAML/DXContainerEmitter.cpp:105
 }
 void DXContainerWriter::writeParts(raw_ostream &OS) {
   uint32_t RollingOffset =
----------------
nit: can you add an empty line in between functions?


================
Comment at: llvm/lib/ObjectYAML/DXContainerEmitter.cpp:121
 
-    // TODO: Write Part data
+    if (P.Name == "DXIL" && P.Program) {
+      dxbc::ProgramHeader Header;
----------------
Consider an early exit instead: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code


================
Comment at: llvm/lib/ObjectYAML/DXContainerEmitter.cpp:153-154
+              Header.Bitcode.Offset - sizeof(dxbc::BitcodeHeader);
+          std::vector<uint8_t> FillData(PadBytes, 0);
+          OS.write(reinterpret_cast<char *>(FillData.data()), PadBytes);
+        }
----------------
Can we use `OS.write_zeros(PadBytes)` instead?


================
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, 
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126795