[PATCH] D131807: [DIrectX backend] emit metadata for entry.

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

You have some stub code in here to handle emitting shader flags, but the value is always set to 0 resulting in nothing being written and thus no tests.

Either we should remove the shader flags code entirely from this change, or we should read the flag value and pass them through. We do have support for some shader flags.



================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:97
+  } CS;
+  EntryProps(Function &F) {
+    Attribute EntryAttr = F.getFnAttribute("hlsl.shader");
----------------
nit: please add a blank line here


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:100
+    StringRef EntryProfile = EntryAttr.getValueAsString();
+    Triple T("", "", "", EntryProfile);
+    ShaderKind = T.getEnvironment();
----------------
We should be able to read the triple from the module and override the environment with the value from the attribute only if the module's environment is library. That saves re-parsing the string unnecessarily.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:111
+      for (auto It : Zip) {
+        auto Str = std::get<0>(It);
+        APInt V;
----------------
nit: IMO, using `auto` as the type from a `std::get` assignment is pretty unintelligible. Can you please make this and the one below explicit types.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:113
+        APInt V;
+        if (Str.getAsInteger(10, V))
+          assert(0 && "invalid num thread");
----------------
The branch here seems odd, a `[[maybe_unused]]` return result is clearer.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:121
+  }
+  MDTuple *emitDXILEntryProps(uint64_t RawShaderFlag, LLVMContext &Ctx,
+                              bool IsLib) {
----------------
nit: please add whitespace between functions


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:140
+private:
+  enum EntryPropsTag {
+    DXILShaderFlagsTag = 0,
----------------
I know you're only handling three possible values of this enum, but we should just move all the cases over. We can also lose the `DXIL` prefix on them since they're all in the DirectX backend DXIL metadata objects already.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:145
+  };
+  void appendNumThreads(std::vector<Metadata *> &MDVals, LLVMContext &Ctx) {
+    MDVals.emplace_back(ConstantAsMetadata::get(
----------------
nit: space between functions.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:155
+  }
+  void appendShaderFlags(std::vector<Metadata *> &MDVals,
+                         uint64_t RawShaderFlag, LLVMContext &Ctx) {
----------------
nit: space between functions


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:162
+  }
+  void appendShaderKind(std::vector<Metadata *> &MDVals, LLVMContext &Ctx) {
+    MDVals.emplace_back(ConstantAsMetadata::get(
----------------
nit: space between functions


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:177
+  EntryMD(Function &F) : F(F), Ctx(F.getContext()), Props(F) {}
+  MDTuple *emitEntryTuple(MDTuple *Resources, uint64_t RawShaderFlag) {
+    // FIXME: add signature for profile other than CS.
----------------
nit: space between functions.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:185
+  }
+  MDTuple *emitEntryTupleForLib(uint64_t RawShaderFlag) {
+    // FIXME: add signature for profile other than CS.
----------------
nit: space between functions.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:191
+        &F, F.getName().str(), Signatures,
+        /*entry in lib doesn't need resources metadta*/ nullptr,
+        Props.emitDXILEntryProps(RawShaderFlag, Ctx, /*IsLib*/ true), Ctx);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131807