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

[Chris Bieneman via Phabricator via llvm-commits]

beanz 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());
+
----------------
kuhar wrote:
> kuhar wrote:
> > 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.
> There's also `arrayRefFromStringRef` in `StringExtras.h`, but I'm not 100% sure if non-const `ArrayRef` is convertible to const-typed one.
`raw_string_ostream` is unbuffered, so flushing does nothing.

Will address the other feedback in an updated patch.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILWriterPass.cpp:80
+    appendToCompilerUsed(M, {GV});
+    return false;
+  }
----------------
kuhar wrote:
> 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.
Yea… I should probably just make it return true. I was thinking that the change wasn’t semantically important, but returning `true` is probably the better approach.


================
Comment at: llvm/test/CodeGen/DirectX/embed-dxil.ll:1
+; RUN: llc %s --filetype=asm -o - | FileCheck %s
+target triple = "dxil-unknown-unknown"
----------------
python3kgae wrote:
> Could this be an opt test?
> Once https://reviews.llvm.org/D124805 is in, we'll have opt work for passes which name started with dxil-. 
> Maybe we can change the pass name to dxil-embed?
Yea, making it an opt test might be better. I also might change the patch to not run the pass as part of the codegen pipeline for asm output because it is less important there. Once I get the MCStreamer and ASMPrinters up this should all come together nicely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125334