[PATCH] D122082: Add DXIL Bitcode Writer and DXIL testing

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:106
+
+/// WriteTypeTable - Write out the type table for a module.
+void DXILBitcodeWriter::writeTypeTable() {
----------------
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments 
Don’t duplicate function or class name at the beginning of the comment.


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:166
+  // Loop over all of the types, emitting each in turn.
+  for (unsigned i = 0, e = TypeList.size(); i != e; ++i) {
+    Type *T = TypeList[i];
----------------
range-based for


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:1689
+
+  // FIXME: Set up the abbrev, we know how many values there are!
+  // FIXME: We know if the type names can use 7-bit ascii.
----------------
The FIXME (copied from BitcodeWriter.cpp) is probably unrelated and should be deleted.

BitcodeWriter.cpp seems to (ab)use 64 inline elements a lot. Does DXIL need 64?
You may consider `SmallVector<unsigned>`.


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:1700
+       SI != SE; ++SI) {
+    SortedTable.push_back(&(*SI));
+  }
----------------
ranged-based for


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:1702
+  }
+  // The keys are unique, so there shouldn't be stability issues
+  std::sort(SortedTable.begin(), SortedTable.end(),
----------------



================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:1705
+            [](const ValueName *A, const ValueName *B) {
+              return (*A).first() < (*B).first();
+            });
----------------



================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:1711
+// HLSL Change - End
+#if 0 // HLSL Change
+  for (ValueSymbolTable::const_iterator SI = VST.begin(), SE = VST.end();
----------------
Just remove `#if 0`. If you need comments, use `//`


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:1878
+    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 8));
+    if (Stream.EmitBlockInfoAbbrev(bitc::VALUE_SYMTAB_BLOCK_ID,
+                                   std::move(Abbv)) != VST_ENTRY_8_ABBREV)
----------------
These llvm_unreachable calls should be assert


================
Comment at: llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp:2092
+  // Emit function bodies.
+  for (Module::const_iterator F = M.begin(), E = M.end(); F != E; ++F)
+    if (!F->isDeclaration())
----------------
range-based for


================
Comment at: llvm/tools/dxil-dis/CMakeLists.txt:8
+
+if (NOT "DirectX" IN_LIST LLVM_EXPERIMENTAL_TARGETS_TO_BUILD)
+  message(FATAL_ERROR "Building dxil-dis tests is unsupported without the DirectX target")
----------------
`LLVM_TARGETS_TO_BUILD` is a superset. I think with this change this line wont' need a change when DirectX becomes non-experimental.


================
Comment at: llvm/tools/dxil-dis/CMakeLists.txt:39
+
+if (CMAKE_HOST_UNIX)
+  set(LLVM_LINK_OR_COPY create_symlink)
----------------
Any reason cmake/modules/AddLLVM.cmake `add_llvm_tool_symlink` can't be used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122082