[DIrectX backend] emit metadata for entry.

[python3kgae]

Summary

New named metadata "dx.entryPoints" is added to save all entries.

Each entry is in format of
!{ptr to function, name, signature, resource table, extra}

For compute shader, the extra will save num of threads in format of {i32 x, i32 y, i32 z}

For library profile, an empty entry will be added to save the resource table for the library.

Signature and resource table metadata is not generated yet.

[python3kgae]

Rebase, update Name for metadata and test.

[beanz]

Rather than copying code from DXC which relies on lots of defined constants, and is very non-LLVM in style, we should focus on adding useful OO abstractions for working with the DXIL metadata.

I refactored the code for writing the dx.valver metadata in D134397. That pattern may not apply to all the metadata we need to generate, but we should break away from what DXC did.

[python3kgae]

Rebase for DXILMetadata.h/cpp

[python3kgae]

In D131807#3809994, @beanz wrote:

Rather than copying code from DXC which relies on lots of defined constants, and is very non-LLVM in style, we should focus on adding useful OO abstractions for working with the DXIL metadata.

I refactored the code for writing the dx.valver metadata in D134397. That pattern may not apply to all the metadata we need to generate, but we should break away from what DXC did.

Following the pattern in https://reviews.llvm.org/D134469 for dx.shaderModel.

[python3kgae]

Refactor code to make it easier to understand.

[python3kgae]

Cleanup header

[beanz]

Please break the testcase updates where you are just changing the triple out into a separate change, and feel free to land it without review.

I have a lot of little bits of feedback, but I'm about to post my change for dx.resources, so we can look at how these two will come together.

Not as part of this change, but at some point soon I want to refactor some of this code into a new library under llvm/lib/Frontend, so that we can share some of this code between the backend and Clang to reduce duplication.

llvm/lib/Target/DirectX/DXILMetadata.cpp

22 ↗

(On Diff #462778)

These function names don't conform to LLVM coding standards:

https://releases.llvm.org/8.0.0/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Function names should be verb phrases that are imperative. I specifically removed this when I refactored the dx.valver change because it didn't conform to the coding standards and I felt that this abstraction made the code harder to read.

If we want to have a shorthand method for creating metadata, we should probably add new APIs to the IRBuilder, or other existing LLVM types rather than adding static methods like this.

49 ↗

(On Diff #462778)

This is a pattern that comes from DXC which we shouldn't bring with us upstream.

Using constant variables for every random constant value was important for vending DXIL headers, but that isn't something we're going to support out of the LLVM codebase. The downside to this pattern is that you either need to view code in an IDE with hover over to tell you the value, or you need to constantly jump back and forth to understand the code. There isn't really much of an upside because our coding standards require tests, so any misspellings should be caught in test cases.

58 ↗

(On Diff #462778)

I question what benefit we get from this being a union, but since there is only one type of shader we're supporting initially, I think you should just have a struct that is the compute shader properties.

70 ↗

(On Diff #462778)

Prefer StringRef::getAsInteger because it returns error conditions.

90 ↗

(On Diff #462778)

The places where these tags are used in DXIL are metadata nodes that store key-value pairs. The tags should be an enumeration, not a local constant.

145 ↗

(On Diff #462778)

I don't see how this enumeration is clearer than just having the indexes since it is just used right below.

148 ↗

(On Diff #462778)

Can this be called if Fn is null? That seems like a bad state to be in.

178 ↗

(On Diff #462778)

Rather than a default case we could enumerate each of the other profiles, and rather than an unreachable an assert makes more sense.

[python3kgae]

Code cleanup to match comments.

[python3kgae]

In D131807#3816334, @beanz wrote:

Please break the testcase updates where you are just changing the triple out into a separate change, and feel free to land it without review.

I have a lot of little bits of feedback, but I'm about to post my change for dx.resources, so we can look at how these two will come together.

Not as part of this change, but at some point soon I want to refactor some of this code into a new library under llvm/lib/Frontend, so that we can share some of this code between the backend and Clang to reduce duplication.

Will create separate change for the triple only test update.
For 'dx.resource', the plan is to add entry md after 'dx.resources' is added. Then M.getNamedMetadata('dx.resources') to find the named MD created when create 'dx.resources'.

llvm/lib/Target/DirectX/DXILMetadata.cpp

148 ↗

(On Diff #462778)

That happens on the library profile when accessing the resource table from the entry metadata.

We should remove it for future shader model.
For older shader models, if driver only read resource table from the "dx.resources" metadata, we could remove it too. Not sure if anyone depends on the current format.

[python3kgae]

Rebase and read resources from "dx.resources".

[python3kgae]

Rebase and fix new tests.

[python3kgae]

Fix newline before enable auto newline in VSCode.

[python3kgae]

Rebase

[python3kgae]

Remove left constexpr StringLiteral.
Use llvm::zip to avoid index variable.
Use Env - Pixel to get shader kind.

[beanz]

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.

llvm/lib/Target/DirectX/DXILMetadata.cpp

97 ↗

(On Diff #482221)

nit: please add a blank line here

100 ↗

(On Diff #482221)

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.

111 ↗

(On Diff #482221)

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.

113 ↗

(On Diff #482221)

The branch here seems odd, a [[maybe_unused]] return result is clearer.

121 ↗

(On Diff #482221)

nit: please add whitespace between functions

140 ↗

(On Diff #482221)

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.

145 ↗

(On Diff #482221)

nit: space between functions.

155 ↗

(On Diff #482221)

nit: space between functions

162 ↗

(On Diff #482221)

nit: space between functions

177 ↗

(On Diff #482221)

nit: space between functions.

185 ↗

(On Diff #482221)

nit: space between functions.

191 ↗

(On Diff #482221)

[python3kgae]

Add ShaderFlags.
Not emit resource table when no resource.
Also fix nit style issues.

[beanz]

LGTM

llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp

static uint32_t ConstMDToUint32(const MDOperand &MDO) {
   return (uint32_t)pConst->getZExtValue();
 }
 
 static ConstantAsMetadata *Uint32ToConstMD(unsigned v, LLVMContext &Ctx) {
   return ConstantAsMetadata::get(
       Constant::getIntegerValue(IntegerType::get(Ctx, 32), APInt(32, v)));
 }
 
+static ConstantAsMetadata *Uint64ToConstMD(uint64_t V, LLVMContext &Ctx) {
+  return ConstantAsMetadata::get(
+      Constant::getIntegerValue(IntegerType::get(Ctx, 64), APInt(64, V)));
+}
+
 constexpr StringLiteral ValVerKey = "dx.valver";
 constexpr unsigned DXILVersionNumFields = 2;
 
 static void emitDXILValidatorVersion(Module &M, VersionTuple &ValidatorVer) {
   NamedMDNode *DXILValidatorVersionMD = M.getNamedMetadata(ValVerKey);
 
   // Allow re-writing the validator version, since this can be changed at
   // later points.
   if (DXILValidatorVersionMD)
     M.eraseNamedMetadata(DXILValidatorVersionMD);
 
   DXILValidatorVersionMD = M.getOrInsertNamedMetadata(ValVerKey);
 
   auto &Ctx = M.getContext();
   Metadata *MDVals[DXILVersionNumFields];
   MDVals[0] = Uint32ToConstMD(ValidatorVer.getMajor(), Ctx);
   MDVals[1] = Uint32ToConstMD(ValidatorVer.getMinor().value_or(0), Ctx);
 
   DXILValidatorVersionMD->addOperand(MDNode::get(Ctx, MDVals));
 }
 
+namespace {
+struct DxilFunctionProps {
+  Triple::EnvironmentType ShaderKind;
+  union {
+    // Compute shader.
+    struct {
+      unsigned NumThreads[3];
+    } CS;
+  } ShaderProps;
+};
+
+const unsigned DXILEntryPointNumFields = 5;
+// Entry point function symbol.
+const unsigned DXILEntryPointFunction = 0;
+// Entry point unmangled name.
+const unsigned DXILEntryPointName = 1;
+// Entry point signature tuple.
+const unsigned DXILEntryPointSignatures = 2;
+// Entry point resource tuple.
+const unsigned DXILEntryPointResources = 3;
+// Entry point properties tuple.
+const unsigned DXILEntryPointProperties = 4;
+} // namespace
+
+static MDTuple *emitDXILEntryProps(DxilFunctionProps &Props, LLVMContext &Ctx,
+                                   uint64_t RawShaderFlag) {
+  std::vector<Metadata *> MDVals;
+
+  if (RawShaderFlag != 0) {
+    static const unsigned DxilShaderFlagsTag = 0;
+    MDVals.emplace_back(Uint32ToConstMD(DxilShaderFlagsTag, Ctx));
+    MDVals.emplace_back(Uint64ToConstMD(RawShaderFlag, Ctx));
+  }
+
+  if (Props.ShaderKind == Triple::EnvironmentType::Compute) {
+    auto &CS = Props.ShaderProps.CS;
+    static const unsigned DXILNumThreadsTag = 4;
+    MDVals.emplace_back(Uint32ToConstMD(DXILNumThreadsTag, Ctx));
+
+    std::vector<Metadata *> NumThreadVals;
+    NumThreadVals.emplace_back(Uint32ToConstMD(CS.NumThreads[0], Ctx));
+    NumThreadVals.emplace_back(Uint32ToConstMD(CS.NumThreads[1], Ctx));
+    NumThreadVals.emplace_back(Uint32ToConstMD(CS.NumThreads[2], Ctx));
+    MDVals.emplace_back(MDNode::get(Ctx, NumThreadVals));
+  }
+
+  return MDNode::get(Ctx, MDVals);
+}
+
+static MDTuple *emitDxilEntryPointTuple(Function *Fn, const std::string &Name,
+                                        MDTuple *Signatures, MDTuple *Resources,
+                                        MDTuple *Properties, LLVMContext &Ctx) {
+  Metadata *MDVals[DXILEntryPointNumFields];
+  MDVals[DXILEntryPointFunction] = Fn ? ValueAsMetadata::get(Fn) : nullptr;
+  MDVals[DXILEntryPointName] = MDString::get(Ctx, Name.c_str());
+  MDVals[DXILEntryPointSignatures] = Signatures;
+  MDVals[DXILEntryPointResources] = Resources;
+  MDVals[DXILEntryPointProperties] = Properties;
+  return MDNode::get(Ctx, MDVals);
+}
+
+static uint64_t collectShaderFlags(Module &M) {
+  uint64_t Flags = 0;
+  // FIXME: build shader flags.
+  return Flags;
+}
+
+static void emitDXILEntry(Module &M) {
+  SmallVector<std::pair<Function *, DxilFunctionProps>> EntryList;
+  constexpr StringLiteral DXILEntryKey = "dx.shader";
+  for (auto &F : M.functions()) {
+    if (!F.hasFnAttribute(DXILEntryKey))
+      continue;
+    Attribute EntryAttr = F.getFnAttribute(DXILEntryKey);
+    StringRef EntryProfile = EntryAttr.getValueAsString();
+    Triple T("", "", "", EntryProfile);
+    DxilFunctionProps Props;
+    Props.ShaderKind = T.getEnvironment();
+    if (Props.ShaderKind == Triple::EnvironmentType::Compute) {
+      constexpr StringLiteral NumThreadsKey = "dx.numthreads";
+      auto NumThreadsStr = F.getFnAttribute(NumThreadsKey).getValueAsString();
+      SmallVector<StringRef> NumThreads;
+      NumThreadsStr.split(NumThreads, ',');
+      assert(NumThreads.size() == 3 && "invalid numthreads");
+      Props.ShaderProps.CS.NumThreads[0] = std::stoi(NumThreads[0].str());
+      Props.ShaderProps.CS.NumThreads[1] = std::stoi(NumThreads[1].str());
+      Props.ShaderProps.CS.NumThreads[2] = std::stoi(NumThreads[2].str());
+    }
+    EntryList.emplace_back(std::make_pair(&F, Props));
+  }
+
+  auto &Ctx = M.getContext();
+  // FIXME: generate metadata for resource.
+  MDTuple *MDResources = nullptr;
+
+  uint64_t ShaderFlags = collectShaderFlags(M);
+  std::vector<MDNode *> Entries;
+
+  Triple T = Triple(M.getTargetTriple());
+  switch (T.getEnvironment()) {
+  default:
+    llvm_unreachable("invalid profile");
+    break;
+  case Triple::EnvironmentType::Library: {
+    // Add empty entry to put resource metadata.
+    MDTuple *EmptyEntry =
+        emitDxilEntryPointTuple(nullptr, "", nullptr, nullptr, nullptr, Ctx);
+    Entries.emplace_back(EmptyEntry);
+  } break;
+  // FIXME: support more shader profiles.
+  case Triple::EnvironmentType::Compute:
+    assert(EntryList.size() == 1 &&
+           "non-lib profiles should only have one entry");
+    break;
+  }
+
+  for (auto &It : EntryList) {
+    Function *F = It.first;
+    DxilFunctionProps &Props = It.second;
+    MDTuple *MDProps = emitDXILEntryProps(Props, Ctx, ShaderFlags);
+    // FIXME add signature for profile other than CS.
+    MDTuple *Sig = nullptr;
+
+    MDTuple *SubEntry = emitDxilEntryPointTuple(F, F->getName().str(), Sig,
+                                                nullptr, MDProps, Ctx);
+
+    Entries.emplace_back(SubEntry);
+  }
+
+  // Set resource for first entry.
+  Entries.front()->replaceOperandWith(DXILEntryPointResources, MDResources);
+
+  constexpr StringLiteral DXILEntryPointsMDName = "dx.entryPoints";
+  NamedMDNode *EntryPointsNamedMD =
+      M.getOrInsertNamedMetadata(DXILEntryPointsMDName);
+  for (auto *Entry : Entries) {
+    EntryPointsNamedMD->addOperand(Entry);
+  }
+}
+
 static VersionTuple loadDXILValidatorVersion(MDNode *ValVerMD) {
   if (ValVerMD->getNumOperands() != DXILVersionNumFields)
     return VersionTuple();
 
   unsigned Major = ConstMDToUint32(ValVerMD->getOperand(0));
   unsigned Minor = ConstMDToUint32(ValVerMD->getOperand(1));
   return VersionTuple(Major, Minor);
 }

 
 bool DXILTranslateMetadata::runOnModule(Module &M) {
   if (MDNode *ValVerMD = cast_or_null<MDNode>(M.getModuleFlag(ValVerKey))) {
     auto ValVer = loadDXILValidatorVersion(ValVerMD);
     if (!ValVer.empty())
       ValidatorVer = ValVer;
   }
   emitDXILValidatorVersion(M, ValidatorVer);
+  emitDXILEntry(M);
   cleanModule(M);
   return false;
 }
 
 char DXILTranslateMetadata::ID = 0;
 
 ModulePass *llvm::createDXILTranslateMetadataPass() {
   return new DXILTranslateMetadata();
 }
 
 INITIALIZE_PASS(DXILTranslateMetadata, "dxil-metadata-emit",
                 "DXIL Metadata Emit", false, false)

llvm/test/CodeGen/DirectX/empty_cs_entry.ll

+; RUN: opt -S -dxil-metadata-emit < %s | FileCheck %s
+target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
+target triple = "dxil-unknown-shadermodel6.7-compute"
+
+;CHECK:!dx.entryPoints = !{![[entry:[0-9]+]]}
+
+;CHECK:![[entry]] = !{ptr @entry, !"entry", null, null, ![[extra:[0-9]+]]}
+;CHECK:![[extra]] = !{i32 4, ![[numthreads:[0-9]+]]}
+;CHECK:![[numthreads]] = !{i32 1, i32 2, i32 1}
+
+; Function Attrs: noinline nounwind
+define void @entry() #0 {
+entry:
+  ret void
+}
+
+attributes #0 = { noinline nounwind "dx.numthreads"="1,2,1" "dx.shader"="compute" }

llvm/test/CodeGen/DirectX/lib_entry.ll

+; RUN: opt -S -dxil-metadata-emit < %s | FileCheck %s
+target datalayout = "e-m:e-p:32:32-i1:32-i8:8-i16:16-i32:32-i64:64-f16:16-f32:32-f64:64-n8:16:32:64"
+target triple = "dxil-unknown-shadermodel6.7-library"
+
+;CHECK:!dx.entryPoints = !{![[empty_entry:[0-9]+]], ![[entry:[0-9]+]]}
+
+; Make sure generate empty entry for lib profile.
+;CHECK![[empty_entry]] = !{null, !"", null, null, null}
+
+;CHECK:![[entry]] = !{ptr @entry, !"entry", null, null, ![[extra:[0-9]+]]}
+;CHECK:![[extra]] = !{i32 4, ![[numthreads:[0-9]+]]}
+;CHECK:![[numthreads]] = !{i32 1, i32 2, i32 1}
+
+; Function Attrs: noinline nounwind
+define void @entry() #0 {
+entry:
+  ret void
+}
+
+attributes #0 = { noinline nounwind "dx.numthreads"="1,2,1" "dx.shader"="compute" }