[DirectX][NFC] Refactor `DXILRootSignature` to follow the same pattern as other analysis #146783

joaosaffran · 2025-07-02T21:13:01Z

When implementing #146785, notice DXILRootSignature had some design changes that made it harder to integrate with other analysis. This change refactors DXILRootSignature to solve this issue.

This is achieved by: 1) Preventing existing dependent analysis of running; 2) Adding RootSignatureBindingInfo class to hold the result of analysis; 3) Adding getRSInfo, other analysis have similar methods to access their results.

llvmbot · 2025-07-02T21:13:34Z

@llvm/pr-subscribers-backend-directx

Author: None (joaosaffran)

Changes

As part of #126645, DXILRootSignature analysis needs to be referenced in multiple passes, which can lead to duplication of data. To fix that, this patch makes sure the analysis only parses the data once.

Full diff: https://github.com/llvm/llvm-project/pull/146783.diff

3 Files Affected:

(modified) llvm/lib/Target/DirectX/DXContainerGlobals.cpp (+4-5)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.cpp (+9-5)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.h (+37-12)

diff --git a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
index 9c38901f6821f..6c8ae8eaaea77 100644
--- a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
+++ b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
@@ -160,18 +160,17 @@ void DXContainerGlobals::addRootSignature(Module &M,
 
   assert(MMI.EntryPropertyVec.size() == 1);
 
-  auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>();
+  auto &RSA = getAnalysis<RootSignatureAnalysisWrapper>().getRSInfo();
   const Function *EntryFunction = MMI.EntryPropertyVec[0].Entry;
-  const auto &FuncRs = RSA.find(EntryFunction);
+  const auto &RS = RSA.getDescForFunction(EntryFunction);
 
-  if (FuncRs == RSA.end())
+  if (!RS)
     return;
 
-  const RootSignatureDesc &RS = FuncRs->second;
   SmallString<256> Data;
   raw_svector_ostream OS(Data);
 
-  RS.write(OS);
+  RS->write(OS);
 
   Constant *Constant =
       ConstantDataArray::getString(M.getContext(), Data, /*AddNull*/ false);
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.cpp b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
index 29e78fcce5262..5a53ea8a3631b 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.cpp
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.cpp
@@ -554,9 +554,12 @@ analyzeModule(Module &M) {
 
 AnalysisKey RootSignatureAnalysis::Key;
 
-SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>
+RootSignatureAnalysis::Result
 RootSignatureAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
-  return analyzeModule(M);
+  if (!AnalysisResult)
+    AnalysisResult = std::make_unique<RootSignatureBindingInfo>(
+        RootSignatureBindingInfo(analyzeModule(M)));
+  return *AnalysisResult;
 }
 
 //===----------------------------------------------------------------------===//
@@ -564,8 +567,7 @@ RootSignatureAnalysis::run(Module &M, ModuleAnalysisManager &AM) {
 PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
                                                     ModuleAnalysisManager &AM) {
 
-  SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc> &RSDMap =
-      AM.getResult<RootSignatureAnalysis>(M);
+  RootSignatureBindingInfo &RSDMap = AM.getResult<RootSignatureAnalysis>(M);
 
   OS << "Root Signature Definitions"
      << "\n";
@@ -636,7 +638,9 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M,
 
 //===----------------------------------------------------------------------===//
 bool RootSignatureAnalysisWrapper::runOnModule(Module &M) {
-  FuncToRsMap = analyzeModule(M);
+  if (!FuncToRsMap)
+    FuncToRsMap = std::make_unique<RootSignatureBindingInfo>(
+        RootSignatureBindingInfo(analyzeModule(M)));
   return false;
 }
 
diff --git a/llvm/lib/Target/DirectX/DXILRootSignature.h b/llvm/lib/Target/DirectX/DXILRootSignature.h
index b45cebc15fd39..8a057404e01c1 100644
--- a/llvm/lib/Target/DirectX/DXILRootSignature.h
+++ b/llvm/lib/Target/DirectX/DXILRootSignature.h
@@ -33,17 +33,46 @@ enum class RootSignatureElementKind {
   CBV = 5,
   DescriptorTable = 6,
 };
+
+class RootSignatureBindingInfo {
+  private:
+    SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc> FuncToRsMap;
+
+  public:
+  using iterator =
+        SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>::iterator;
+
+  RootSignatureBindingInfo () = default;
+  RootSignatureBindingInfo(SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc> Map) : FuncToRsMap(Map) {};
+
+  iterator find(const Function *F) { return FuncToRsMap.find(F); }
+
+  iterator end() { return FuncToRsMap.end(); }
+
+  std::optional<mcdxbc::RootSignatureDesc> getDescForFunction(const Function* F) {
+    const auto FuncRs = find(F);
+    if (FuncRs == end())
+      return std::nullopt;
+
+    return FuncRs->second;
+  }
+  
+};
+
 class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
   friend AnalysisInfoMixin<RootSignatureAnalysis>;
   static AnalysisKey Key;
 
 public:
-  RootSignatureAnalysis() = default;
 
-  using Result = SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>;
+RootSignatureAnalysis() = default;
+
+  using Result = RootSignatureBindingInfo;
+
+  Result run(Module &M, ModuleAnalysisManager &AM);
 
-  SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>
-  run(Module &M, ModuleAnalysisManager &AM);
+private:
+  std::unique_ptr<RootSignatureBindingInfo> AnalysisResult;
 };
 
 /// Wrapper pass for the legacy pass manager.
@@ -52,20 +81,16 @@ class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
 /// passes which run through the legacy pass manager.
 class RootSignatureAnalysisWrapper : public ModulePass {
 private:
-  SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc> FuncToRsMap;
+  std::unique_ptr<RootSignatureBindingInfo> FuncToRsMap;
 
 public:
   static char ID;
+  using Result = RootSignatureBindingInfo;
 
   RootSignatureAnalysisWrapper() : ModulePass(ID) {}
 
-  using iterator =
-      SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>::iterator;
-
-  iterator find(const Function *F) { return FuncToRsMap.find(F); }
-
-  iterator end() { return FuncToRsMap.end(); }
-
+  RootSignatureBindingInfo& getRSInfo() {return *FuncToRsMap;}
+  
   bool runOnModule(Module &M) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;

github-actions · 2025-07-02T21:15:38Z

✅ With the latest revision this PR passed the C/C++ code formatter.

spall · 2025-07-07T14:48:03Z

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

  SmallString<256> Data;
  raw_svector_ostream OS(Data);

-  RS.write(OS);
+  RS->write(OS);


isn't RS a reference, why are you using the arrow?

Updated the type to make it clearer, but the type is const std::optional<mcdxbc::RootSignatureDesc> &RS

spall · 2025-07-07T14:56:46Z

llvm/lib/Target/DirectX/DXILRootSignature.h

 class RootSignatureAnalysis : public AnalysisInfoMixin<RootSignatureAnalysis> {
  friend AnalysisInfoMixin<RootSignatureAnalysis>;
  static AnalysisKey Key;

 public:
  RootSignatureAnalysis() = default;

-  using Result = SmallDenseMap<const Function *, mcdxbc::RootSignatureDesc>;
+  using Result = RootSignatureBindingInfo;


Why create a type alias here? Why not just put 'RootSignatureBindingInfo' inline? It looks like its only used one place.

Result type alias is required in analysis passes or passes that can return something, like in this case, it can contain the Root Signature data representation.

spall · 2025-07-07T14:58:03Z

llvm/lib/Target/DirectX/DXILRootSignature.h


 public:
  static char ID;
+  using Result = RootSignatureBindingInfo;


This type alias doesn't even appear to be used?

bogner

This change isn't sound. If we're running the analysis pass again, that means that the pass manager has said it's been invalidated, and if it's been invalidated then it isn't safe to return the previous result.

What we actually need to do here is either (1) ensure that passes that don't modify the metadata don't invalidate this pass, or (2) make this an immutable pass. I suspect (1) is more correct - if passes were to modify the root signature metadata they would indeed invalidate this pass.

inbelic

Could we update the description to explain why the refactor allows us to easier integrate?

Is it just to provide the helper class? Or does the change from addRequired -> addPresevered help integration?

inbelic · 2025-07-10T16:54:37Z

llvm/lib/Target/DirectX/DXILRootSignature.h

@@ -83,3 +106,4 @@ class RootSignatureAnalysisPrinter

 } // namespace dxil
 } // namespace llvm
+#endif


nit:

Suggested change

#endif

#endif // LLVM_LIB_TARGET_DIRECTX_DXILROOTSIGNATURE_H

inbelic · 2025-07-10T17:04:48Z

llvm/lib/Target/DirectX/DXILRootSignature.cpp

-  AU.addRequired<DXILMetadataAnalysisWrapperPass>();
+  AU.addPreserved<DXILMetadataAnalysisWrapperPass>();


Is this a NFC?

Yes, changing from addRequired to addPreserved means we will not run the analysis, we will just use existing analysis results. Since that Analysis already run in the pipeline, there is no actual logic change here.

Okay, thanks for updating the description

joaosaffran added 3 commits June 26, 2025 19:34

refactoring

0e8828c

refactoring

2edd215

clean up

242545e

llvmbot added the backend:DirectX label Jul 2, 2025

joaosaffran added 4 commits July 2, 2025 21:22

format

3f8dec4

formating

3b1ce3b

fix import issues

f5720af

formating

ea54904

spall reviewed Jul 7, 2025

View reviewed changes

joaosaffran added 2 commits July 7, 2025 20:14

address comments

881dd36

format

c7d5be7

joaosaffran requested review from spall, inbelic and bogner July 8, 2025 17:18

joaosaffran assigned bogner and inbelic Jul 8, 2025

bogner requested changes Jul 8, 2025

View reviewed changes

bogner mentioned this pull request Jul 8, 2025

[DirectX] Validate registers are bound to root signature #146785

Open

address changes

cc5afae

joaosaffran requested a review from bogner July 8, 2025 20:55

joaosaffran changed the title ~~[DirectX] Update DXILRootSignature to not run analysis after data has been parsed~~ [DirectX][NFC] RefactorDXILRootSignature to follow the same pattern as other analysis Jul 9, 2025

joaosaffran changed the title ~~[DirectX][NFC] RefactorDXILRootSignature to follow the same pattern as other analysis~~ [DirectX][NFC] Refactor DXILRootSignature to follow the same pattern as other analysis Jul 9, 2025

bogner approved these changes Jul 9, 2025

View reviewed changes

inbelic reviewed Jul 10, 2025

View reviewed changes

joaosaffran merged commit ca888f0 into llvm:main Jul 10, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DirectX][NFC] Refactor `DXILRootSignature` to follow the same pattern as other analysis #146783

[DirectX][NFC] Refactor `DXILRootSignature` to follow the same pattern as other analysis #146783

Uh oh!

joaosaffran commented Jul 2, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025 •

edited

Loading

Uh oh!

spall Jul 7, 2025

Uh oh!

joaosaffran Jul 7, 2025

Uh oh!

spall Jul 7, 2025

Uh oh!

joaosaffran Jul 7, 2025

Uh oh!

spall Jul 7, 2025

Uh oh!

bogner left a comment •

edited

Loading

Uh oh!

inbelic left a comment

Uh oh!

inbelic Jul 10, 2025 •

edited

Loading

Uh oh!

inbelic Jul 10, 2025

Uh oh!

joaosaffran Jul 10, 2025

Uh oh!

inbelic Jul 10, 2025

Uh oh!

Uh oh!

Uh oh!

		AU.addRequired<DXILMetadataAnalysisWrapperPass>();
		AU.addPreserved<DXILMetadataAnalysisWrapperPass>();

[DirectX][NFC] Refactor DXILRootSignature to follow the same pattern as other analysis #146783

[DirectX][NFC] Refactor DXILRootSignature to follow the same pattern as other analysis #146783

Uh oh!

Conversation

joaosaffran commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inbelic left a comment

Choose a reason for hiding this comment

Uh oh!

inbelic Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[DirectX][NFC] Refactor `DXILRootSignature` to follow the same pattern as other analysis #146783

[DirectX][NFC] Refactor `DXILRootSignature` to follow the same pattern as other analysis #146783

joaosaffran commented Jul 2, 2025 •

edited

Loading

github-actions bot commented Jul 2, 2025 •

edited

Loading

bogner left a comment •

edited

Loading

inbelic Jul 10, 2025 •

edited

Loading