[PATCH] D125334: [DirectX] Embed DXIL in LLVM Module

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILWriterPass.cpp:69-70
+
+    ArrayRef<uint8_t> ModuleData =
+        ArrayRef<uint8_t>((const uint8_t *)Data.data(), Data.size());
+
----------------
1. Are we sure that `WriteDXILToFile` flushes the stream before returning?
2. We don't need the assignment, you can do `ArrayRef<uint8_t> X(A, B)` to run the constructor that takes two pointers/.
3. Use `reinterpret_cast` instead of C-style casts.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILWriterPass.cpp:74
+        ConstantDataArray::get(M.getContext(), ModuleData);
+    GlobalVariable *GV =
+        new llvm::GlobalVariable(M, ModuleConstant->getType(), true,
----------------
nit: the type if obvious from the RHS, so we can use `auto *GV`


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILWriterPass.cpp:80
+    appendToCompilerUsed(M, {GV});
+    return false;
+  }
----------------
Can you add a brief comment on why we return `false`? It appears like the module might be modified by one of the function calls above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125334