[DXIL] Add support for root signature flag element in DXContainer #123147
Conversation
joaosaffran
force-pushed
the
rootsignature/flags-metadata
branch
from
ad9a49f to
273db6e
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-objectyaml @llvm/pr-subscribers-backend-directx Author: None (joaosaffran) ChangesAdding support for Root Signature Flags Element extraction and writing to DXContainer.
Full diff: https://github.com/llvm/llvm-project/pull/123147.diff 7 Files Affected:
diff --git a/llvm/lib/Target/DirectX/CMakeLists.txt b/llvm/lib/Target/DirectX/CMakeLists.txt
index 26315db891b577..89fe494dea71cc 100644
--- a/llvm/lib/Target/DirectX/CMakeLists.txt
+++ b/llvm/lib/Target/DirectX/CMakeLists.txt
@@ -33,7 +33,7 @@ add_llvm_target(DirectXCodeGen
DXILResourceAccess.cpp
DXILShaderFlags.cpp
DXILTranslateMetadata.cpp
-
+ DXILRootSignature.cpp
LINK_COMPONENTS
Analysis
AsmPrinter
diff --git a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
index 7a0bd6a7c88692..ac70bd3771dadf 100644
--- a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
+++ b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
@@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//
+#include "DXILRootSignature.h"
#include "DXILShaderFlags.h"
#include "DirectX.h"
#include "llvm/ADT/SmallVector.h"
@@ -26,6 +27,7 @@
#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 +43,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 +63,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 +77,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 +149,23 @@ void DXContainerGlobals::addSignature(Module &M,
Globals.emplace_back(buildSignature(M, OutputSig, "dx.osg1", "OSG1"));
}
+void DXContainerGlobals::addRootSignature(Module &M,
+ SmallVector<GlobalValue *> &Globals) {
+
+ std::optional<ModuleRootSignature> MRS =
+ getAnalysis<RootSignatureAnalysisWrapper>().getRootSignature();
+ if (!MRS.has_value())
+ return;
+
+ SmallString<256> Data;
+ raw_svector_ostream OS(Data);
+ MRS->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();
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
new file mode 100644
index 00000000000000..cabaec3671078e
--- /dev/null
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -0,0 +1,147 @@
+//===- 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/BinaryFormat/DXContainer.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/Metadata.h"
+#include "llvm/IR/Module.h"
+#include <cassert>
+
+using namespace llvm;
+using namespace llvm::dxil;
+
+static bool parseRootFlags(ModuleRootSignature *MRS, MDNode *RootFlagNode) {
+
+ assert(RootFlagNode->getNumOperands() == 2 &&
+ "Invalid format for RootFlag Element");
+ auto *Flag = mdconst::extract<ConstantInt>(RootFlagNode->getOperand(1));
+ auto Value = Flag->getZExtValue();
+
+ // Root Element validation, as specified:
+ // https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#validations-during-dxil-generation
+ assert((Value & ~0x80000fff) != 0 && "Invalid flag for RootFlag Element");
+
+ MRS->Flags = Value;
+ return false;
+}
+
+static bool parseRootSignatureElement(ModuleRootSignature *MRS,
+ MDNode *Element) {
+ MDString *ElementText = cast<MDString>(Element->getOperand(0));
+ assert(ElementText != nullptr &&
+ "First preoperty of element is not a string");
+
+ RootSignatureElementKind ElementKind =
+ StringSwitch<RootSignatureElementKind>(ElementText->getString())
+ .Case("RootFlags", RootSignatureElementKind::RootFlags)
+ .Case("RootConstants", RootSignatureElementKind::RootConstants)
+ .Case("RootCBV", RootSignatureElementKind::RootDescriptor)
+ .Case("RootSRV", RootSignatureElementKind::RootDescriptor)
+ .Case("RootUAV", RootSignatureElementKind::RootDescriptor)
+ .Case("Sampler", RootSignatureElementKind::RootDescriptor)
+ .Case("DescriptorTable", RootSignatureElementKind::DescriptorTable)
+ .Case("StaticSampler", RootSignatureElementKind::StaticSampler)
+ .Default(RootSignatureElementKind::None);
+
+ switch (ElementKind) {
+
+ case RootSignatureElementKind::RootFlags: {
+ return parseRootFlags(MRS, Element);
+ break;
+ }
+
+ case RootSignatureElementKind::RootConstants:
+ case RootSignatureElementKind::RootDescriptor:
+ case RootSignatureElementKind::DescriptorTable:
+ case RootSignatureElementKind::StaticSampler:
+ case RootSignatureElementKind::None:
+ llvm_unreachable("Not Implemented yet");
+ break;
+ }
+
+ return true;
+}
+
+bool ModuleRootSignature::parse(int32_t Version, NamedMDNode *Root) {
+ this->Version = Version;
+ bool HasError = false;
+
+ for (unsigned int Sid = 0; Sid < Root->getNumOperands(); Sid++) {
+ // This should be an if, for error handling
+ MDNode *Node = cast<MDNode>(Root->getOperand(Sid));
+
+ // Not sure what use this for...
+ // Metadata *Func = Node->getOperand(0).get();
+
+ MDNode *Elements = cast<MDNode>(Node->getOperand(1).get());
+ assert(Elements && "Invalid Metadata type on root signature");
+
+ for (unsigned int Eid = 0; Eid < Elements->getNumOperands(); Eid++) {
+ MDNode *Element = cast<MDNode>(Elements->getOperand(Eid));
+ assert(Element && "Invalid Metadata type on root element");
+
+ HasError = HasError || parseRootSignatureElement(this, Element);
+ }
+ }
+ return HasError;
+}
+
+void ModuleRootSignature::write(raw_ostream &OS) {
+ dxbc::RootSignatureDesc Out{this->Version, this->Flags};
+
+ if (sys::IsBigEndianHost) {
+ Out.swapBytes();
+ }
+
+ OS.write(reinterpret_cast<const char *>(&Out),
+ sizeof(dxbc::RootSignatureDesc));
+}
+
+AnalysisKey RootSignatureAnalysis::Key;
+
+ModuleRootSignature RootSignatureAnalysis::run(Module &M,
+ ModuleAnalysisManager &AM) {
+ ModuleRootSignature MRSI;
+
+ NamedMDNode *RootSignatureNode = M.getNamedMetadata("dx.rootsignatures");
+ if (RootSignatureNode) {
+ MRSI.parse(1, RootSignatureNode);
+ }
+
+ return MRSI;
+}
+
+//===----------------------------------------------------------------------===//
+bool RootSignatureAnalysisWrapper::runOnModule(Module &M) {
+ ModuleRootSignature MRS;
+
+ NamedMDNode *RootSignatureNode = M.getNamedMetadata("dx.rootsignatures");
+ if (RootSignatureNode) {
+ MRS.parse(1, RootSignatureNode);
+ this->MRS = MRS;
+ }
+
+ return false;
+}
+
+void RootSignatureAnalysisWrapper::getAnalysisUsage(AnalysisUsage &AU) const {
+ AU.setPreservesAll();
+}
+
+char RootSignatureAnalysisWrapper::ID = 0;
+
+INITIALIZE_PASS(RootSignatureAnalysisWrapper, "dx-root-signature-analysis",
+ "DXIL Root Signature Analysis", true, true)
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.h b/llvm/lib/Target/DirectX/DXILRootSignature.h
new file mode 100644
index 00000000000000..de82afcdc8c467
--- /dev/null
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.h
@@ -0,0 +1,74 @@
+//===- DXILRootSignature.h - 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 "llvm/IR/Metadata.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/Pass.h"
+#include <optional>
+
+namespace llvm {
+namespace dxil {
+
+enum class RootSignatureElementKind {
+ None = 0,
+ RootFlags = 1,
+ RootConstants = 2,
+ RootDescriptor = 3,
+ DescriptorTable = 4,
+ StaticSampler = 5
+};
+
+struct ModuleRootSignature {
+ uint32_t Version;
+ uint32_t Flags;
+
+ ModuleRootSignature() = default;
+
+ bool parse(int32_t Version, NamedMDNode *Root);
+ void write(raw_ostream &OS);
+};
+
+class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
+ friend AnalysisInfoMixin<RootSignatureAnalysis>;
+ static AnalysisKey Key;
+
+public:
+ RootSignatureAnalysis() = default;
+
+ using Result = ModuleRootSignature;
+
+ ModuleRootSignature run(Module &M, ModuleAnalysisManager &AM);
+};
+
+/// Wrapper pass for the legacy pass manager.
+///
+/// This is required because the passes that will depend on this are codegen
+/// passes which run through the legacy pass manager.
+class RootSignatureAnalysisWrapper : public ModulePass {
+ std::optional<ModuleRootSignature> MRS;
+
+public:
+ static char ID;
+
+ RootSignatureAnalysisWrapper() : ModulePass(ID) {}
+
+ const std::optional<ModuleRootSignature> &getRootSignature() { return MRS; }
+
+ bool runOnModule(Module &M) override;
+
+ void getAnalysisUsage(AnalysisUsage &AU) const override;
+};
+
+} // namespace dxil
+} // namespace llvm
diff --git a/llvm/lib/Target/DirectX/DirectX.h b/llvm/lib/Target/DirectX/DirectX.h
index add23587de7d58..953ac3eb820987 100644
--- a/llvm/lib/Target/DirectX/DirectX.h
+++ b/llvm/lib/Target/DirectX/DirectX.h
@@ -77,6 +77,9 @@ void initializeDXILPrettyPrinterLegacyPass(PassRegistry &);
/// Initializer for dxil::ShaderFlagsAnalysisWrapper pass.
void initializeShaderFlagsAnalysisWrapperPass(PassRegistry &);
+/// Initializer for dxil::RootSignatureAnalysisWrapper pass.
+void initializeRootSignatureAnalysisWrapperPass(PassRegistry &);
+
/// Initializer for DXContainerGlobals pass.
void initializeDXContainerGlobalsPass(PassRegistry &);
diff --git a/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp b/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
index ecb1bf775f8578..93745d7a5cb0d2 100644
--- a/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
+++ b/llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
@@ -61,6 +61,7 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeDirectXTarget() {
initializeDXILTranslateMetadataLegacyPass(*PR);
initializeDXILResourceMDWrapperPass(*PR);
initializeShaderFlagsAnalysisWrapperPass(*PR);
+ initializeRootSignatureAnalysisWrapperPass(*PR);
initializeDXILFinalizeLinkageLegacyPass(*PR);
}
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Flags.ll b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Flags.ll
new file mode 100644
index 00000000000000..ffbf5e9ffd1d32
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Flags.ll
@@ -0,0 +1,38 @@
+; RUN: opt %s -dxil-embed -dxil-globals -S -o - | FileCheck %s
+; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC
+
+target triple = "dxil-unknown-shadermodel6.0-compute"
+
+; CHECK: @dx.rts0 = private constant [8 x i8] c"{{.*}}", section "RTS0", align 4
+
+
+define void @main() #0 {
+entry:
+ ret void
+}
+
+attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" }
+
+
+!dx.rootsignatures = !{!2} ; list of function/root signature pairs
+!2 = !{ ptr @main, !3 } ; function, root signature
+!3 = !{ !4 } ; list of root signature elements
+!4 = !{ !"RootFlags", i32 1 } ; 1 = allow_input_assembler_input_layout
+
+
+; DXC: - Name: RTS0
+; DXC-NEXT: Size: 8
+; DXC-NEXT: RootSignature:
+; DXC-NEXT: Version: 1
+; DXC-NEXT: AllowInputAssemblerInputLayout: true
+; DXC-NEXT: DenyVertexShaderRootAccess: false
+; DXC-NEXT: DenyHullShaderRootAccess: false
+; DXC-NEXT: DenyDomainShaderRootAccess: false
+; DXC-NEXT: DenyGeometryShaderRootAccess: false
+; DXC-NEXT: DenyPixelShaderRootAccess: false
+; DXC-NEXT: AllowStreamOutput: false
+; DXC-NEXT: LocalRootSignature: false
+; DXC-NEXT: DenyAmplificationShaderRootAccess: false
+; DXC-NEXT: DenyMeshShaderRootAccess: false
+; DXC-NEXT: CBVSRVUAVHeapDirectlyIndexed: false
+; DXC-NEXT: SamplerHeapDirectlyIndexed: false
|
joaosaffran
force-pushed
the
rootsignature/flags-metadata
branch
from
6e75dd5 to
01b59db
Compare
joaosaffran
force-pushed
the
rootsignature/flags-metadata
branch
from
01b59db to
789c9a5
Compare
joaosaffran
force-pushed
the
rootsignature/flags-metadata
branch
from
789c9a5 to
3d346c9
Compare
llvm/test/CodeGen/DirectX/ContainerData/RootSignature-MultipleEntryFunctions-Error.ll
Outdated
Show resolved
Hide resolved
llvm/test/CodeGen/DirectX/ContainerData/RootSignature-MultipleEntryFunctions-Error.ll
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We should add a couple of tests that use opt and print the analysis results rather than the llc and obj2yaml. These will be easier to write and uderstand. We should probably modify most of the error tests to do this as well to avoid running other unrelated passes before and after the root signature analysis.
There's a piece that I hadn't noticed you'd missed that's needed to do this though - we need to register the analysis in the new pass manager and also write a printer pass. You can take a look at DXILShaderFlags for the boilerplate for a printer pass, and it may be as simple as:
PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
ModuleAnalysisManager &AM) {
std::optional<ModuleRootSignature> &MRS =
AM.getResult<RootSignatureAnalysis>(M);
if (MRS)
OS << "Root Signature flags: " << format_hex(MRS->Flags, 8) << "\n";
else
OS << "Root Signature: not found";
return PreservedAnalyses::all();
}though we may want to print something a bit more human readable :)
For the registry, this is a matter of adding a MODULE_ANALYSIS("dxil-root-signature", ... and MODULE_PASS("print<dxil-root-signature>", ... to DirectXPassRegistry.def
| // needed to stop compilation | ||
| report_fatal_error("Invalid Root Signature Definition", false); |
There was a problem hiding this comment.
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.
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.
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.
| // needed to stop compilation | ||
| report_fatal_error("Invalid Root Signature Definition", false); |
There was a problem hiding this comment.
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?
| return false; | ||
| } | ||
|
|
||
| static const Function *getEntryFunction(Module &M, ModuleMetadataInfo MMI) { |
There was a problem hiding this comment.
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) { |
| const MDOperand &FunctionPointerMdNode = Node->getOperand(0); | ||
| if (FunctionPointerMdNode == nullptr) { | ||
| // Function was pruned during compilation. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
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.
| static std::optional<ModuleRootSignature> analyzeModule(Module &M, | ||
| const Function *F); |
There was a problem hiding this comment.
This doesn't really need to be public API like this - better to just have a static analyzeModule function in DXILRootSignature.cpp rather than a static member of the ModuleRootSignature struct.
There was a problem hiding this comment.
What is this test trying to cover? There's only one entry function here since @main does not have "hlsl.shader" metadata, but if you change it to @main #0 then we'll obviously get the error like in the -Error version of this test
| @@ -0,0 +1,27 @@ | |||
| ; RUN: not llc %s --filetype=obj -o - 2>&1 | FileCheck %s | |||
There was a problem hiding this comment.
This test crashes in DXContainerGlobals in a way that's unrelated to your pass, which is probably why you were having the issues that led you to add report_fatal_error.
It would be easier to write this test using opt, like not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s - see my overall comment for more on that.
| char RootSignatureAnalysisWrapper::ID = 0; | ||
|
|
||
| INITIALIZE_PASS_BEGIN(RootSignatureAnalysisWrapper, | ||
| "dx-root-signature-analysis", |
There was a problem hiding this comment.
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.
llvm/lib/Object/DXContainer.cpp
Outdated
| return E; | ||
| Version = MaybeVersion.get(); | ||
| if (dxbc::RootSignatureValidations::validateVersion(VValue)) | ||
| return make_error<GenericBinaryError>("Invalid Root Signature Version"); |
There was a problem hiding this comment.
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):
| return make_error<GenericBinaryError>("Invalid Root Signature Version"); | |
| return make_error<GenericBinaryError>(Twine("Unsupported root signature version read: " + VValue); |
llvm/lib/Object/DXContainer.cpp
Outdated
| return E; | ||
| Flags = MaybeFlag.get(); | ||
| if (dxbc::RootSignatureValidations::validateRootFlag(FValue)) | ||
| return make_error<GenericBinaryError>("Invalid Root Signature flag"); |
There was a problem hiding this comment.
Same feedback as above, we can make this a useful error.
| @@ -0,0 +1,91 @@ | |||
| //===- DXILRootSignature.h - DXIL Root Signature helper objects | |||
| //---------------===// | |||
| @@ -0,0 +1,196 @@ | |||
| //===- DXILRootSignature.cpp - DXIL Root Signature helper objects ----===// | |||
There was a problem hiding this comment.
I think I counted this right
| //===- DXILRootSignature.cpp - DXIL Root Signature helper objects ----===// | |
| //===- DXILRootSignature.cpp - DXIL Root Signature helper objects -------===// |
| return parseRootFlags(Ctx, MRS, Element); | ||
| case RootSignatureElementKind::None: | ||
| return reportError(Ctx, | ||
| "Invalid Root Element: " + ElementText->getString()); |
There was a problem hiding this comment.
nit: let's be clear about what we're talking about.
| "Invalid Root Element: " + ElementText->getString()); | |
| "Invalid Root Signature Element: " + ElementText->getString()); |
| if (FunctionPointerMdNode == nullptr) { | ||
| // Function was pruned during compilation. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
| if (FunctionPointerMdNode == nullptr) { | |
| // Function was pruned during compilation. | |
| continue; | |
| } | |
| // Function was pruned during compilation. | |
| if (FunctionPointerMdNode == nullptr) | |
| continue; |
| dxil::ModuleMetadataInfo &MMI = | ||
| getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata(); |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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"
| assert(MMI.EntryPropertyVec.size() == 1); | ||
|
|
||
| auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>(); | ||
| const Function *&EntryFunction = MMI.EntryPropertyVec[0].Entry; |
There was a problem hiding this comment.
A const ref to a pointer is pretty strange. Is this what we really want here? I suspect just copying the pointer is fine.
| bool hasForFunction(const Function *F) { return MRS.find(F) != MRS.end(); } | ||
|
|
||
| ModuleRootSignature getForFunction(const Function *F) { | ||
| assert(hasForFunction(F)); | ||
| return MRS[F]; | ||
| } |
There was a problem hiding this comment.
This API is much more expensive than it should be - using hasForFunction followed by getForFunction has to do the lookup twice (and three times in asserts). I think we just want to wrap SmallDenseMap::find here, no?
| RootSignatureHeader RSH; | ||
| RSH.Flags = MRS.Flags; |
There was a problem hiding this comment.
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....
| ElementText->getString()); | ||
| } | ||
|
|
||
| llvm_unreachable("Root signature element kind not expected."); |
There was a problem hiding this comment.
Wording nit: This isn't "unexpected" - it should be impossible if folks are reading compiler warnings. I might say "Unhandled RootSignatureElementKind enum" here.
| // Function was pruned during compilation. | ||
| const MDOperand &FunctionPointerMdNode = RSDefNode->getOperand(0); | ||
| if (FunctionPointerMdNode == nullptr) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Space++; | ||
| printSpaces(OS, Space); | ||
| OS << "Flags: " << format_hex(MRS.Flags, 8) << ":\n"; | ||
| Space--; |
There was a problem hiding this comment.
I think you can use OS << indent(1) << format_hex(... here instead of printSpaces
| target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
|
||
| ; CHECK: error: Invalid format for Root Signature Definition. Pairs of function, root signature expected. | ||
| ; CHECK-NO: Root Signature Definitions |
There was a problem hiding this comment.
typo? CHECK-NO doesn't do anything.
| ; CHECK-NO: Root Signature Definitions | |
| ; CHECK-NOT: Root Signature Definitions |
this comes up in a few of the tests.
|
|
||
| DXILRootSignature.cpp | ||
| LINK_COMPONENTS |
There was a problem hiding this comment.
Please keep the blank line between the sources and the LINK_COMPONENTS line - it makes this easier to read.
| std::optional<mcdxbc::RootSignatureDesc> getForFunction(const Function *F) { | ||
| auto Lookup = FuncToRsMap.find(F); | ||
| if (Lookup == FuncToRsMap.end()) | ||
| return std::nullopt; | ||
| return Lookup->second; | ||
| } |
There was a problem hiding this comment.
I guess this is ok, but like I said in my previous comment I do think this would all be clearer if we just implement a container interface here. Something like this:
using iterator = SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>::iterator;
iterator find(const Function *F) { return FuncToRsMap.find(F); }
iterator end() { return FuncToRsMap.end(); }There was a problem hiding this comment.
Sorry, misunderstood the previous comment. I will implement this change.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2289 Here is the relevant piece of the build log for the reference |
…vm#123147) Adding support for Root Signature Flags Element extraction and writing to DXContainer. - Adding an analysis to deal with RootSignature metadata definition - Adding validation for Flag - writing RootSignature blob into DXIL Closes: [126632](llvm#126632) --------- Co-authored-by: joaosaffran <[email protected]>
| if (RootElementListNode == nullptr) { | ||
| reportError(Ctx, "Missing Root Element List Metadata node."); | ||
| } |
There was a problem hiding this comment.
Should there be a continue here? It looks to me like we'll go ahead and use a nullptr later if this happens.
…vm#123147) Adding support for Root Signature Flags Element extraction and writing to DXContainer. - Adding an analysis to deal with RootSignature metadata definition - Adding validation for Flag - writing RootSignature blob into DXIL Closes: [126632](llvm#126632) --------- Co-authored-by: joaosaffran <[email protected]>
Adding support for Root Signature Flags Element extraction and writing to DXContainer.
Closes: 126632