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

Chris Bieneman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 26 14:56:13 PDT 2022


beanz added a comment.

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.



================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:22
 
+static ConstantAsMetadata *uint32ToConstMD(unsigned V, LLVMContext &Ctx) {
+  return ConstantAsMetadata::get(ConstantInt::get(Type::getInt32Ty(Ctx), V));
----------------
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.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:49
+namespace {
+constexpr StringLiteral DXILEntryKey = "hlsl.shader";
+
----------------
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.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:58
+    } CS;
+  } ShaderProps;
+  EntryProps(Function &F) {
----------------
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.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:70
+      assert(NumThreads.size() == 3 && "invalid numthreads");
+      ShaderProps.CS.NumThreads[0] = std::stoi(NumThreads[0].str());
+      ShaderProps.CS.NumThreads[1] = std::stoi(NumThreads[1].str());
----------------
Prefer `StringRef::getAsInteger` because it returns error conditions.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:90
+  void appendNumThreads(std::vector<Metadata *> &MDVals, LLVMContext &Ctx) {
+    constexpr unsigned DXILNumThreadsTag = 4;
+    MDVals.emplace_back(uint32ToConstMD(DXILNumThreadsTag, Ctx));
----------------
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.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:145
+      EntryMDProps,
+      EntryMDSize
+    };
----------------
I don't see how this enumeration is clearer than just having the indexes since it is just used right below.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:148
+    Metadata *MDVals[EntryMDSize];
+    MDVals[EntryMDFn] = Fn ? ValueAsMetadata::get(Fn) : nullptr;
+    MDVals[EntryMDName] = MDString::get(Ctx, Name.c_str());
----------------
Can this be called if `Fn` is null? That seems like a bad state to be in.


================
Comment at: llvm/lib/Target/DirectX/DXILMetadata.cpp:178
+  switch (T.getEnvironment()) {
+  default:
+    llvm_unreachable("invalid profile");
----------------
Rather than a default case we could enumerate each of the other profiles, and rather than an `unreachable` an `assert` makes more sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131807



More information about the llvm-commits mailing list