[PATCH] D125158: [DirectX backend] Add pass to emit dxil metadata.

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

Two super nitpick points off the top:
(1) Please be consistent that DXIL is all capitalized. I'd like LLVM to not have the annoyance issues that DXC's codebase has where DXIL is inconsistently capitalized.
(2) Can we instead name the pass "DXILTranslateMetadata"? The pass doesn't actually emit the metadata, it is translating it from one form in the module to another.



================
Comment at: llvm/lib/Target/DirectX/DxilEmitMetadata.cpp:23
+  return (unsigned)pConst->getZExtValue();
+}
+ConstantAsMetadata *Uint32ToConstMD(unsigned v, LLVMContext &Ctx) {
----------------
File-scoped functions should be `static`:

https://www.llvm.org/docs/CodingStandards.html#anonymous-namespaces

Also, your function says it is converting to a 32-bit unsigned integer, but the return type is `unsigned` (which is usually but not guaranteed to be 32-bits), use `uint32_t` instead.

nit: Uint32->UInt32 (unsigned int is two words not one)


================
Comment at: llvm/lib/Target/DirectX/DxilEmitMetadata.cpp:31
+const unsigned DxilVersionMajorIdx = 0; // DXIL version major.
+const unsigned DxilVersionMinorIdx = 1; // DXIL version minor.
+
----------------
Stylistically, I don't love declaring these constants. They're not used in a bunch of places and I feel like version tuples are well enough understood that we should be able to intuit the format.

That said, you don't need to change it.


================
Comment at: llvm/lib/Target/DirectX/DxilEmitMetadata.cpp:92
+
+void DxilEmitMetadata::emitDXILVersion(Module &M) {}
+
----------------
This is unused and does nothing. Should it be here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125158