-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[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
Changes from 66 commits
8adb678
ba78f21
0a54559
557075f
e0d3dcd
80587dd
be3764d
7582ca6
6aaa0a5
916b2f1
d9bce0a
e7676ed
a0cee57
1e7a1fe
0ed658a
932062e
628937c
1378c9f
e3206c9
f93d42d
25e3f37
751cbdc
44532d6
ca21878
987901c
0fbe900
b771aea
aabdfe7
4f6f941
a7f7784
bf3b2e0
16b4d03
e043370
57bf935
1f8c0a5
0905b83
77e8544
1351fb0
09e645a
5a44b62
d1a79b3
9f8e512
5c7ed7e
93f7c4c
5aac761
47b01f7
486ab88
852ac25
74f7226
7a82fa9
c67d039
e80ef33
ef1ce8a
b7f2716
83b0979
b175b65
01b49a7
7bba9d3
2809c2f
023dcb8
aedb446
39e60c0
3b135c6
ae3d03f
3d19f8b
5a3be7c
f64c608
a5a2093
5b3fedc
605f225
e4bca2b
825673f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,11 +254,9 @@ 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 make_error<GenericBinaryError>("Invalid Root Signature Version"); | ||
| Version = VValue; | ||
|
|
||
| NumParameters = | ||
| support::endian::read<uint32_t, llvm::endianness::little>(Current); | ||
|
|
@@ -280,11 +278,9 @@ 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 make_error<GenericBinaryError>("Invalid Root Signature flag"); | ||
|
||
| Flags = FValue; | ||
|
|
||
| return Error::success(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ add_llvm_target(DirectXCodeGen | |
| DXILResourceAccess.cpp | ||
| DXILShaderFlags.cpp | ||
| DXILTranslateMetadata.cpp | ||
|
|
||
| DXILRootSignature.cpp | ||
| LINK_COMPONENTS | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "DXILRootSignature.h" | ||
| #include "DXILShaderFlags.h" | ||
| #include "DirectX.h" | ||
| #include "llvm/ADT/SmallVector.h" | ||
|
|
@@ -23,9 +24,11 @@ | |
| #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/Transforms/Utils/ModuleUtils.h" | ||
| #include <optional> | ||
|
|
||
| using namespace llvm; | ||
| using namespace llvm::dxil; | ||
|
|
@@ -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); | ||
|
|
@@ -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>(); | ||
|
|
@@ -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; | ||
|
|
@@ -144,6 +150,28 @@ void DXContainerGlobals::addSignature(Module &M, | |
| Globals.emplace_back(buildSignature(M, OutputSig, "dx.osg1", "OSG1")); | ||
| } | ||
|
|
||
| void DXContainerGlobals::addRootSignature(Module &M, | ||
| SmallVector<GlobalValue *> &Globals) { | ||
|
|
||
| auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>(); | ||
|
|
||
| if (!RSA.getResult()) | ||
| return; | ||
|
|
||
| const ModuleRootSignature &MRS = RSA.getResult().value(); | ||
| SmallString<256> Data; | ||
| raw_svector_ostream OS(Data); | ||
|
|
||
| RootSignatureHeader RSH; | ||
| RSH.Flags = MRS.Flags; | ||
|
||
|
|
||
| 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(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,215 @@ | ||||||||||||||||
| //===- DXILRootSignature.cpp - DXIL Root Signature helper objects ----===// | ||||||||||||||||
|
||||||||||||||||
| //===- DXILRootSignature.cpp - DXIL Root Signature helper objects ----===// | |
| //===- DXILRootSignature.cpp - DXIL Root Signature helper objects -------===// |
Outdated
There was a problem hiding this comment.
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"
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's be clear about what we're talking about.
| "Invalid Root Element: " + ElementText->getString()); | |
| "Invalid Root Signature Element: " + ElementText->getString()); |
Outdated
There was a problem hiding this comment.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you construct a test case that includes this? Is this something that can actually happen - a function is referenced by some metadata, and the function is removed but not the metadata that references it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this error when running test: RootSignature-MultipleEntryFunctions.ll. During root signature generation, function @main was null, since entry point was function @anotherMain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that normal, or is this an indication of a bug somewhere where we also need to strip the corresponding metadata when a function is pruned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems expected, not used functions will be removed, since we record function pointers in the metadata, if the function is removed the pointer will be null. @bogner please correct me.
I think we should clean it, but we might need to do it in another pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be able to get here without hand-written invalid IR. The RootSignature-MultipleEntryFunctions.ll doesn't have multiple entry functions - it has entry function metadata for functions that are not entry functions - since @main doesn't have hlsl.shader metadata and is not exported it's removed by DXILFinalizeLinkage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (FunctionPointerMdNode == nullptr) { | |
| // Function was pruned during compilation. | |
| continue; | |
| } | |
| // Function was pruned during compilation. | |
| if (FunctionPointerMdNode == nullptr) | |
| continue; |
damyanp marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to copy or modify the MMI here.
| static const Function *getEntryFunction(Module &M, ModuleMetadataInfo MMI) { | |
| static const Function *getEntryFunction(Module &M, const ModuleMetadataInfo &MMI) { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is concerning and looks wrong. We shouldn't be crashing here when we're using diagnostics to report errors - we should fail gracefully. LLVM's diagnostic machinery will stop the compilation and bubble the error state up to the driver (which will then generally error out in whatever way it sees fit).
It might be better to get rid of getEntryFunction and move this logic into analyzeModule, where it's a bit more obvious how to fail gracefully rather than just return a nullptr that'll cause problems later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention is that analyzeModule gets the root signature for a specific function defined inside a module. This is useful, since we have some scenarios where we will need to get multiple root signatures from multiple entry functions, mainly lib shaders.
I agree that it is having the getEntryFunction makes the error handling more confusing. One solution I think it is best here is to modify the RootSignatureAnalysis to return as Result a Map<Function*, RootSignature>. This way we can reuse this analysis in other scenarios that might require getting root signatures, and we can move the decision of which RootSignature to get to another pass. In this specific scenario, the logic can go to DXContainerGlobals, it can do something like:
dxil::ModuleMetadataInfo &MMI =
getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata();
assert(MMI.EntryPropertyVec.size() == 1 ||
MMI.ShaderProfile == Triple::Library);
auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>();
const ModuleRootSignature &MRS = RSA.getForFunction(MMI.EntryPropertyVec[0].Entry);@bogner thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to refactor this so that the analysis result is different I suppose that's fine, but all I'm saying is that analyzeModule is the simplest place to deal with the error handling here. Whether it has a signature of static std::optional<ModuleRootSignature> analyzeModule(Module &M, const ModuleMetadataInfo &MMI) or static DenseMap<Function *, ModuleRootSignature> analyzeModule(Module &M, ...) doesn't really affect my comment AFAICT.
damyanp marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - why would we need to stop compilation here? Shouldn't returning std::nullopt do the right thing and allow us to let the pass machinery error out normally?
damyanp marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called dxil-root-signature-analysis. The dx- prefix seems to be used in DXILShaderFlags for some reason, but generally we've been consistent with the dxil- prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These error strings are what get printed by LLVM if we ever hit this error condition. This message doesn't tell me what the actual error is.
Maybe something like (needs proper formatting):