This is an archive of the discontinued LLVM Phabricator instance.

[DIrectX backend] emit metadata for entry.
ClosedPublic

Authored by python3kgae on Aug 12 2022, 2:00 PM.

Details

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.

Event Timeline

python3kgae created this revision.Aug 12 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 2:00 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
python3kgae requested review of this revision.Aug 12 2022, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 2:00 PM

Rebase, update Name for metadata and test.

beanz added a comment.Sep 22 2022, 2:47 PM

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.

Rebase for DXILMetadata.h/cpp

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.

Refactor code to make it easier to understand.

Cleanup header

beanz added a comment.Sep 26 2022, 2:56 PM

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
23

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.

44

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.

53

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.

65

Prefer StringRef::getAsInteger because it returns error conditions.

85

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.

140

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

143

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

173

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

python3kgae marked 8 inline comments as done.

Code cleanup to match comments.

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
143

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.

Rebase and read resources from "dx.resources".

Rebase and fix new tests.

Fix newline before enable auto newline in VSCode.

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

beanz added a comment.Dec 15 2022, 9:17 AM

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
56

nit: please add a blank line here

59

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.

70

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.

72

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

80

nit: please add whitespace between functions

99

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.

104

nit: space between functions.

114

nit: space between functions

121

nit: space between functions

136

nit: space between functions.

144

nit: space between functions.

150
python3kgae marked 11 inline comments as done.

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

python3kgae marked an inline comment as done.Dec 15 2022, 3:26 PM
beanz accepted this revision.Dec 21 2022, 11:36 AM

LGTM

This revision is now accepted and ready to land.Dec 21 2022, 11:36 AM
This revision was automatically updated to reflect the committed changes.