Skip to content

[DXIL] Add support for root signature flag element in DXContainer #123147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 72 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from 69 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
8adb678
adding rootsignature to obj2yaml
Jan 13, 2025
ba78f21
adding test
Jan 13, 2025
0a54559
removing old test
Jan 13, 2025
557075f
remove useless includes
Jan 13, 2025
e0d3dcd
addressing comments
Jan 14, 2025
80587dd
updating test
Jan 14, 2025
be3764d
removing useless header
Jan 15, 2025
7582ca6
fix formating
Jan 15, 2025
6aaa0a5
renaming test
Jan 15, 2025
916b2f1
addressing pr comments
Jan 16, 2025
d9bce0a
adding str to ROOT_ELEMENT_FLAG
Jan 16, 2025
e7676ed
formating
Jan 17, 2025
a0cee57
refactoring to follow llvm standards
Jan 18, 2025
1e7a1fe
addressing comments
Jan 27, 2025
0ed658a
clean up
Jan 27, 2025
932062e
remove version
Jan 30, 2025
628937c
fix pr
Jan 30, 2025
1378c9f
adding dxil-dis test
Jan 31, 2025
e3206c9
adding compatibility test
Jan 31, 2025
f93d42d
addressing test concerns
Feb 3, 2025
25e3f37
clean up
Feb 3, 2025
751cbdc
addressing comments
Feb 4, 2025
44532d6
adding fail test
Feb 4, 2025
ca21878
adding comment
Feb 4, 2025
987901c
adding few more tests
Feb 4, 2025
0fbe900
format
Feb 4, 2025
b771aea
cleanup
Feb 5, 2025
aabdfe7
adding metadata extraction
Jan 15, 2025
4f6f941
moving root signature to it's own pass
Jan 16, 2025
a7f7784
formating
Jan 16, 2025
bf3b2e0
removing useless imports
Jan 16, 2025
16b4d03
fixing pr changes
Jan 16, 2025
e043370
adding some asserts
Jan 16, 2025
57bf935
format
Jan 16, 2025
1f8c0a5
fixing assert
Jan 18, 2025
0905b83
cleaning
Jan 27, 2025
77e8544
clean up
Jan 29, 2025
1351fb0
addressing comments
Jan 30, 2025
09e645a
removing version
Jan 30, 2025
5a44b62
fix test
Jan 30, 2025
d1a79b3
addressing PR Comments
Jan 31, 2025
9f8e512
fix test
Feb 3, 2025
5c7ed7e
filtering root signatures not associated with entry function
Feb 4, 2025
93f7c4c
separating parsing and validation
Feb 4, 2025
5aac761
improve error handling
Feb 6, 2025
47b01f7
clean up
Feb 6, 2025
486ab88
clean up
Feb 6, 2025
852ac25
formating
Feb 6, 2025
74f7226
addressing comments and fix tests
Feb 7, 2025
7a82fa9
fixing tests
Feb 7, 2025
c67d039
formating
Feb 7, 2025
e80ef33
Merge branch 'main' into users/joaosaffran/123147
Feb 7, 2025
ef1ce8a
fix
Feb 7, 2025
b7f2716
Merge branch 'main' into users/joaosaffran/123147
Feb 10, 2025
83b0979
addressing pr comments
Feb 10, 2025
b175b65
addressing PR comments
Feb 11, 2025
01b49a7
addressing pr comments
Feb 11, 2025
7bba9d3
removing copies from root signature use in dx container globals
Feb 11, 2025
2809c2f
adding more tests
Feb 12, 2025
023dcb8
maybe fix test?
Feb 12, 2025
aedb446
fixing clang format
Feb 12, 2025
39e60c0
try fix format
Feb 12, 2025
3b135c6
removing test
Feb 12, 2025
ae3d03f
adding llvm unreachable and testing test
Feb 12, 2025
3d19f8b
stopping compilation if root signature error were emitted
Feb 12, 2025
5a3be7c
making sure Error tests fail
Feb 12, 2025
f64c608
refactoring root signature analysis to return a map instead
Feb 12, 2025
a5a2093
addressing pr comments
Feb 12, 2025
5b3fedc
clean up
Feb 12, 2025
605f225
addressing pr comments
Feb 13, 2025
e4bca2b
implementing find interface for RootSignatureAnalysisWrapper
Feb 13, 2025
825673f
adding test for null function
Feb 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions llvm/include/llvm/BinaryFormat/DXContainer.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
#define LLVM_BINARYFORMAT_DXCONTAINER_H

#include "llvm/ADT/StringRef.h"
#include "llvm/Support/BinaryStreamError.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/SwapByteOrder.h"
#include "llvm/TargetParser/Triple.h"

Expand Down Expand Up @@ -550,18 +548,10 @@ static_assert(sizeof(ProgramSignatureElement) == 32,

struct RootSignatureValidations {

static Expected<uint32_t> validateRootFlag(uint32_t Flags) {
if ((Flags & ~0x80000fff) != 0)
return llvm::make_error<BinaryStreamError>("Invalid Root Signature flag");
return Flags;
}

static Expected<uint32_t> validateVersion(uint32_t Version) {
if (Version == 1 || Version == 2)
return Version;
static bool isValidRootFlag(uint32_t Flags) { return (Flags & ~0xfff) == 0; }

return llvm::make_error<BinaryStreamError>(
"Invalid Root Signature Version");
static bool isValidVersion(uint32_t Version) {
return (Version == 1 || Version == 2);
}
};

Expand Down
22 changes: 12 additions & 10 deletions llvm/lib/Object/DXContainer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ static Error parseFailed(const Twine &Msg) {
return make_error<GenericBinaryError>(Msg.str(), object_error::parse_failed);
}

static Error validationFailed(const Twine &Msg) {
return make_error<StringError>(Msg.str(), inconvertibleErrorCode());
}

template <typename T>
static Error readStruct(StringRef Buffer, const char *Src, T &Struct) {
// Don't read before the beginning or past the end of the file
Expand Down Expand Up @@ -254,11 +258,10 @@ Error DirectX::RootSignature::parse(StringRef Data) {
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

Expected<uint32_t> MaybeVersion =
dxbc::RootSignatureValidations::validateVersion(VValue);
if (Error E = MaybeVersion.takeError())
return E;
Version = MaybeVersion.get();
if (!dxbc::RootSignatureValidations::isValidVersion(VValue))
return validationFailed("unsupported root signature version read: " +
llvm::Twine(VValue));
Version = VValue;

NumParameters =
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Expand All @@ -280,11 +283,10 @@ Error DirectX::RootSignature::parse(StringRef Data) {
support::endian::read<uint32_t, llvm::endianness::little>(Current);
Current += sizeof(uint32_t);

Expected<uint32_t> MaybeFlag =
dxbc::RootSignatureValidations::validateRootFlag(FValue);
if (Error E = MaybeFlag.takeError())
return E;
Flags = MaybeFlag.get();
if (!dxbc::RootSignatureValidations::isValidRootFlag(FValue))
return validationFailed("unsupported root signature flag value read: " +
llvm::Twine(FValue));
Flags = FValue;

return Error::success();
}
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/DirectX/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ add_llvm_target(DirectXCodeGen
DXILResourceAccess.cpp
DXILShaderFlags.cpp
DXILTranslateMetadata.cpp

DXILRootSignature.cpp
LINK_COMPONENTS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the blank line between the sources and the LINK_COMPONENTS line - it makes this easier to read.

Analysis
AsmPrinter
Expand Down
39 changes: 39 additions & 0 deletions llvm/lib/Target/DirectX/DXContainerGlobals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//

#include "DXILRootSignature.h"
#include "DXILShaderFlags.h"
#include "DirectX.h"
#include "llvm/ADT/SmallVector.h"
Expand All @@ -23,8 +24,10 @@
#include "llvm/IR/Module.h"
#include "llvm/InitializePasses.h"
#include "llvm/MC/DXContainerPSVInfo.h"
#include "llvm/MC/DXContainerRootSignature.h"
#include "llvm/Pass.h"
#include "llvm/Support/MD5.h"
#include "llvm/TargetParser/Triple.h"
#include "llvm/Transforms/Utils/ModuleUtils.h"

using namespace llvm;
Expand All @@ -41,6 +44,7 @@ class DXContainerGlobals : public llvm::ModulePass {
GlobalVariable *buildSignature(Module &M, Signature &Sig, StringRef Name,
StringRef SectionName);
void addSignature(Module &M, SmallVector<GlobalValue *> &Globals);
void addRootSignature(Module &M, SmallVector<GlobalValue *> &Globals);
void addResourcesForPSV(Module &M, PSVRuntimeInfo &PSV);
void addPipelineStateValidationInfo(Module &M,
SmallVector<GlobalValue *> &Globals);
Expand All @@ -60,6 +64,7 @@ class DXContainerGlobals : public llvm::ModulePass {
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.setPreservesAll();
AU.addRequired<ShaderFlagsAnalysisWrapper>();
AU.addRequired<RootSignatureAnalysisWrapper>();
AU.addRequired<DXILMetadataAnalysisWrapperPass>();
AU.addRequired<DXILResourceTypeWrapperPass>();
AU.addRequired<DXILResourceBindingWrapperPass>();
Expand All @@ -73,6 +78,7 @@ bool DXContainerGlobals::runOnModule(Module &M) {
Globals.push_back(getFeatureFlags(M));
Globals.push_back(computeShaderHash(M));
addSignature(M, Globals);
addRootSignature(M, Globals);
addPipelineStateValidationInfo(M, Globals);
appendToCompilerUsed(M, Globals);
return true;
Expand Down Expand Up @@ -144,6 +150,39 @@ void DXContainerGlobals::addSignature(Module &M,
Globals.emplace_back(buildSignature(M, OutputSig, "dx.osg1", "OSG1"));
}

void DXContainerGlobals::addRootSignature(Module &M,
SmallVector<GlobalValue *> &Globals) {

dxil::ModuleMetadataInfo &MMI =
getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata();
Comment on lines +156 to +157
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something that needs to be changed for this PR, but accessing analysis results here shows that it's a bit concerning how DXContainer is designed - we'll need to do a lot of work to port it to the new pass manager.


// Root Signature in Library shaders are different,
// since they don't use DXContainer to share it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is confusing. Can you reword it a bit please? It should say why we don't need to handle library shaders here, not simpy point out that they're "different"

if (MMI.ShaderProfile == llvm::Triple::Library)
return;

assert(MMI.EntryPropertyVec.size() == 1);

auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>();
const Function *&EntryFunction = MMI.EntryPropertyVec[0].Entry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A const ref to a pointer is pretty strange. Is this what we really want here? I suspect just copying the pointer is fine.


if (!RSA.hasForFunction(EntryFunction))
return;

const ModuleRootSignature &MRS = RSA.getForFunction(EntryFunction);
SmallString<256> Data;
raw_svector_ostream OS(Data);

RootSignatureHeader RSH;
RSH.Flags = MRS.Flags;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have an auxilliary structure that we simply copy through here? Could RootSignatureAnalysis just provide us with a RootSignatureHeader directly? Is ModuleRootSignature going to grow into something that just duplicates RootSignatureHeader's fields?

aside: Both ModuleRootSignature and RootSignatureHeader feel kind of poorly named - ModuleRootSignature has nothing to do with the module AFAICT, and RootSignatureHeader is the object that derives the whole root signature, not just the header....


RSH.write(OS);

Constant *Constant =
ConstantDataArray::getString(M.getContext(), Data, /*AddNull*/ false);
Globals.emplace_back(buildContainerGlobal(M, Constant, "dx.rts0", "RTS0"));
}

void DXContainerGlobals::addResourcesForPSV(Module &M, PSVRuntimeInfo &PSV) {
const DXILBindingMap &DBM =
getAnalysis<DXILResourceBindingWrapperPass>().getBindingMap();
Expand Down
224 changes: 224 additions & 0 deletions llvm/lib/Target/DirectX/DXILRootSignature.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
//===- DXILRootSignature.cpp - DXIL Root Signature helper objects -------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
///
/// \file This file contains helper objects and APIs for working with DXIL
/// Root Signatures.
///
//===----------------------------------------------------------------------===//
#include "DXILRootSignature.h"
#include "DirectX.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Analysis/DXILMetadataAnalysis.h"
#include "llvm/BinaryFormat/DXContainer.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Metadata.h"
#include "llvm/IR/Module.h"
#include "llvm/InitializePasses.h"
#include "llvm/Pass.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include <cstdint>
#include <optional>
#include <utility>

using namespace llvm;
using namespace llvm::dxil;

static bool reportError(LLVMContext *Ctx, Twine Message,
DiagnosticSeverity Severity = DS_Error) {
Ctx->diagnose(DiagnosticInfoGeneric(Message, Severity));
return true;
}

static bool parseRootFlags(LLVMContext *Ctx, ModuleRootSignature &MRS,
MDNode *RootFlagNode) {

if (RootFlagNode->getNumOperands() != 2)
return reportError(Ctx, "Invalid format for RootFlag Element");

auto *Flag = mdconst::extract<ConstantInt>(RootFlagNode->getOperand(1));
MRS.Flags = Flag->getZExtValue();

return false;
}

static bool parseRootSignatureElement(LLVMContext *Ctx,
ModuleRootSignature &MRS,
MDNode *Element) {
MDString *ElementText = cast<MDString>(Element->getOperand(0));
if (ElementText == nullptr)
return reportError(Ctx, "Invalid format for Root Element");

RootSignatureElementKind ElementKind =
StringSwitch<RootSignatureElementKind>(ElementText->getString())
.Case("RootFlags", RootSignatureElementKind::RootFlags)
.Default(RootSignatureElementKind::None);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should RootSignatureElementKind::None actually be called RootSignatureElementKind::Error? The current name makes me think there is simply no root signature, but the way it's used seems to generally be "the root signature is invalid"


switch (ElementKind) {

case RootSignatureElementKind::RootFlags:
return parseRootFlags(Ctx, MRS, Element);
case RootSignatureElementKind::None:
return reportError(Ctx, "Invalid Root Signature Element: " +
ElementText->getString());
}

llvm_unreachable("Root signature element kind not expected.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wording nit: This isn't "unexpected" - it should be impossible if folks are reading compiler warnings. I might say "Unhandled RootSignatureElementKind enum" here.

}

static bool parse(LLVMContext *Ctx, ModuleRootSignature &MRS, MDNode *Node) {
bool HasError = false;

// Loop through the Root Elements of the root signature.
for (const auto &Operand : Node->operands()) {
MDNode *Element = dyn_cast<MDNode>(Operand);
if (Element == nullptr)
return reportError(Ctx, "Missing Root Element Metadata Node.");

HasError = HasError || parseRootSignatureElement(Ctx, MRS, Element);
}

return HasError;
}

static bool validate(LLVMContext *Ctx, const ModuleRootSignature &MRS) {
if (!dxbc::RootSignatureValidations::isValidRootFlag(MRS.Flags)) {
return reportError(Ctx, "Invalid Root Signature flag value");
}
return false;
}

static SmallDenseMap<const Function *, ModuleRootSignature>
analyzeModule(Module &M) {

/** Root Signature are specified as following in the metadata:

!dx.rootsignatures = !{!2} ; list of function/root signature pairs
!2 = !{ ptr @main, !3 } ; function, root signature
!3 = !{ !4, !5, !6, !7 } ; list of root signature elements

So for each MDNode inside dx.rootsignatures NamedMDNode
(the Root parameter of this function), the parsing process needs
to loop through each of its operands and process the function,
signature pair.
*/

LLVMContext *Ctx = &M.getContext();

SmallDenseMap<const Function *, ModuleRootSignature> MRSMap;

NamedMDNode *RootSignatureNode = M.getNamedMetadata("dx.rootsignatures");
if (RootSignatureNode == nullptr)
return MRSMap;

for (const auto &RSDefNode : RootSignatureNode->operands()) {
if (RSDefNode->getNumOperands() != 2) {
reportError(Ctx, "Invalid format for Root Signature Definition. Pairs "
"of function, root signature expected.");
continue;
}

// Function was pruned during compilation.
const MDOperand &FunctionPointerMdNode = RSDefNode->getOperand(0);
if (FunctionPointerMdNode == nullptr) {
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a reportError, and we should test it. We should not get here normally, and if we do, we were given invalid IR.

(there's an argument that some of these could be asserts, depending on whether we consider invalid IR to be user input or a frontend bug, but for consistency with the other cases here reportError seems the most appropriate)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that a function might be removed, but the root signature metadata associated with it is not? For example, if the user change the Entry function using the CLI flag?

If that is possible, we might need to update DXILFinalizeLinkage to also remove the associated root signature metadata, when pruning functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any way that we could get here from HLSL, no. As far as I can tell the only way we can get to this situation is if we have invalid LLVM IR in the first place.


ValueAsMetadata *VAM =
llvm::dyn_cast<ValueAsMetadata>(FunctionPointerMdNode.get());
if (VAM == nullptr) {
reportError(Ctx, "First element of root signature is not a Value");
continue;
}

Function *F = dyn_cast<Function>(VAM->getValue());
if (F == nullptr) {
reportError(Ctx, "First element of root signature is not a Function");
continue;
}

MDNode *RootElementListNode =
dyn_cast<MDNode>(RSDefNode->getOperand(1).get());

if (RootElementListNode == nullptr) {
reportError(Ctx, "Missing Root Element List Metadata node.");
}
Comment on lines +156 to +158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a continue here? It looks to me like we'll go ahead and use a nullptr later if this happens.


ModuleRootSignature MRS;

if (parse(Ctx, MRS, RootElementListNode) || validate(Ctx, MRS)) {
return MRSMap;
}

MRSMap.insert(std::make_pair(F, MRS));
}

return MRSMap;
}

AnalysisKey RootSignatureAnalysis::Key;

SmallDenseMap<const Function *, ModuleRootSignature>
RootSignatureAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
return analyzeModule(M);
}

//===----------------------------------------------------------------------===//

static void printSpaces(raw_ostream &Stream, unsigned int Count) {
for (unsigned int I = 0; I < Count; ++I) {
Stream << ' ';
}
}

PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
ModuleAnalysisManager &AM) {

SmallDenseMap<const Function *, ModuleRootSignature> &MRSMap =
AM.getResult<RootSignatureAnalysis>(M);
OS << "Root Signature Definitions"
<< "\n";
uint8_t Space = 0;
for (const auto &P : MRSMap) {
const auto &[Function, MRS] = P;
OS << "Definition for '" << Function->getName() << "':\n";

// start root signature header
Space++;
printSpaces(OS, Space);
OS << "Flags: " << format_hex(MRS.Flags, 8) << ":\n";
Space--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use OS << indent(1) << format_hex(... here instead of printSpaces

// end root signature header
}

return PreservedAnalyses::all();
}

//===----------------------------------------------------------------------===//
bool RootSignatureAnalysisWrapper::runOnModule(Module &M) {
MRS = analyzeModule(M);
return false;
}

void RootSignatureAnalysisWrapper::getAnalysisUsage(AnalysisUsage &AU) const {
AU.setPreservesAll();
AU.addRequired<DXILMetadataAnalysisWrapperPass>();
}

char RootSignatureAnalysisWrapper::ID = 0;

INITIALIZE_PASS_BEGIN(RootSignatureAnalysisWrapper,
"dxil-root-signature-analysis",
"DXIL Root Signature Analysis", true, true)
INITIALIZE_PASS_END(RootSignatureAnalysisWrapper,
"dxil-root-signature-analysis",
"DXIL Root Signature Analysis", true, true)
Loading