[PATCH] D131545: [DirectX backend] emit metadata for DXIL version, ShaderModel.

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:54
+  Triple T = Triple(M.getTargetTriple());
+  VersionTuple DXILVer = T.getOSVersion();
+
----------------
This is a bit confusingly named. Probably worth calling this `SMVer` instead and explaining in a comment that the DXIL and SM minor versions are the same, and the major versions are offset.


================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:75
+  // Shader model.
+  static const unsigned DXILShaderModelNumFields = 3;
+  // Shader type (vs,ps,cs,gs,ds,hs).
----------------
nit: Rather than `static const` `constexpr`. The later is guaranteed to be compile-time resolved, the former isn't.


================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:85
+  MDVals[DXILShaderModelTypeIdx] = MDString::get(Ctx, T.getEnvironmentName());
+  auto Ver = T.getOSVersion();
+  auto Minor = Ver.getMinor();
----------------
nit: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp:86
+  auto Ver = T.getOSVersion();
+  auto Minor = Ver.getMinor();
+  MDVals[DXILShaderModelMajorIdx] = Uint32ToConstMD(Ver.getMajor(), Ctx);
----------------
This one really requires you to know the API to know that `getMinor()` returns an `Optional` rather than a value.

Other places in LLVM use a pattern more like:
```
unsigned Minor = V.getMinor().value_or(0);
```

Which would also be more clear.


================
Comment at: llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.h:78
 /// mode.
-void WriteDXILToFile(const Module &M, raw_ostream &Out);
+void WriteDXILToFile(Module &M, raw_ostream &Out);
 
----------------
I know we had talked about where to mutate the triple and I thought `WriteDXILToFile` made sense, but seeing the code now that we lose the const-ness here I'm rethinking.

We only really need the triple to be altered when we're embedding DXIL in the module for emission of a container. I don't even think the `WriteDXILPass` is ever used (unless it were called manually).

It might make more sense to mutate the triple in `EmbedDXILPass::runOnModule` to preserve the const-ness of the writing function. We can also in a subsequent patch delete the `WriteDXILPass`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131545