[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo #107101

python3kgae · 2024-09-03T12:51:48Z

Replace the hardcoded values for compute shader in DXContainer::addPipelineStateValidationInfo.
Still missing wave size.

Add preserved for previous passes so the information is not lost.

Fix llvm/wg-hlsl#51

Replace the hardcoded values for compute shader in DXContainer::addPipelineStateValidationInfo. Still missing wave size. Add preserved for previous passes so the information is not lost. Fix llvm/wg-hlsl#51

llvmbot · 2024-09-03T12:52:06Z

@llvm/pr-subscribers-backend-directx

Author: Xiang Li (python3kgae)

Changes

Replace the hardcoded values for compute shader in DXContainer::addPipelineStateValidationInfo.
Still missing wave size.

Add preserved for previous passes so the information is not lost.

Fix llvm/wg-hlsl#51

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

4 Files Affected:

(modified) llvm/lib/Target/DirectX/DXContainerGlobals.cpp (+19-4)
(modified) llvm/lib/Target/DirectX/DXILPrepare.cpp (+2)
(modified) llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp (+2)
(added) llvm/test/CodeGen/DirectX/ContainerData/RuntimeInfoCS.ll (+41)

diff --git a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
index d47b9c7a25b8fe..1a9e16f102c007 100644
--- a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
+++ b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp
@@ -15,6 +15,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Analysis/DXILMetadataAnalysis.h"
 #include "llvm/BinaryFormat/DXContainer.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/Constants.h"
@@ -57,6 +58,7 @@ class DXContainerGlobals : public llvm::ModulePass {
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
     AU.addRequired<ShaderFlagsAnalysisWrapper>();
+    AU.addRequired<DXILMetadataAnalysisWrapperPass>();
   }
 };
 
@@ -149,15 +151,27 @@ void DXContainerGlobals::addPipelineStateValidationInfo(
   PSV.BaseData.ShaderStage =
       static_cast<uint8_t>(TT.getEnvironment() - Triple::Pixel);
 
+  dxil::ModuleMetadataInfo &MMI =
+      getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata();
+  assert(MMI.EntryPropertyVec.size() != 0 ||
+         TT.getEnvironment() == Triple::Library);
   // Hardcoded values here to unblock loading the shader into D3D.
   //
   // TODO: Lots more stuff to do here!
   //
   // See issue https://github.com/llvm/llvm-project/issues/96674.
-  PSV.BaseData.NumThreadsX = 1;
-  PSV.BaseData.NumThreadsY = 1;
-  PSV.BaseData.NumThreadsZ = 1;
-  PSV.EntryName = "main";
+  switch (TT.getEnvironment()) {
+  case Triple::Compute:
+    PSV.BaseData.NumThreadsX = MMI.EntryPropertyVec[0].NumThreadsX;
+    PSV.BaseData.NumThreadsY = MMI.EntryPropertyVec[0].NumThreadsY;
+    PSV.BaseData.NumThreadsZ = MMI.EntryPropertyVec[0].NumThreadsZ;
+    break;
+  default:
+    break;
+  }
+
+  if (TT.getEnvironment() != Triple::Library)
+    PSV.EntryName = MMI.EntryPropertyVec[0].Entry->getName();
 
   PSV.finalize(TT.getEnvironment());
   PSV.write(OS);
@@ -170,6 +184,7 @@ char DXContainerGlobals::ID = 0;
 INITIALIZE_PASS_BEGIN(DXContainerGlobals, "dxil-globals",
                       "DXContainer Global Emitter", false, true)
 INITIALIZE_PASS_DEPENDENCY(ShaderFlagsAnalysisWrapper)
+INITIALIZE_PASS_DEPENDENCY(DXILMetadataAnalysisWrapperPass)
 INITIALIZE_PASS_END(DXContainerGlobals, "dxil-globals",
                     "DXContainer Global Emitter", false, true)
 
diff --git a/llvm/lib/Target/DirectX/DXILPrepare.cpp b/llvm/lib/Target/DirectX/DXILPrepare.cpp
index 56098864e987fb..f6b7355b936255 100644
--- a/llvm/lib/Target/DirectX/DXILPrepare.cpp
+++ b/llvm/lib/Target/DirectX/DXILPrepare.cpp
@@ -19,6 +19,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Analysis/DXILMetadataAnalysis.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/AttributeMask.h"
 #include "llvm/IR/IRBuilder.h"
@@ -247,6 +248,7 @@ class DXILPrepareModule : public ModulePass {
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addPreserved<ShaderFlagsAnalysisWrapper>();
     AU.addPreserved<DXILResourceMDWrapper>();
+    AU.addPreserved<DXILMetadataAnalysisWrapperPass>();
   }
   static char ID; // Pass identification.
 };
diff --git a/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp b/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
index 2c6d20112060df..701dbc1353dab9 100644
--- a/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
+++ b/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
@@ -14,6 +14,7 @@
 #include "DirectX.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Analysis/DXILResource.h"
+#include "llvm/Analysis/DXILMetadataAnalysis.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
@@ -103,6 +104,7 @@ class DXILTranslateMetadataLegacy : public ModulePass {
     AU.addRequired<DXILResourceWrapperPass>();
     AU.addRequired<DXILResourceMDWrapper>();
     AU.addRequired<ShaderFlagsAnalysisWrapper>();
+    AU.addRequired<DXILMetadataAnalysisWrapperPass>();
   }
 
   bool runOnModule(Module &M) override {
diff --git a/llvm/test/CodeGen/DirectX/ContainerData/RuntimeInfoCS.ll b/llvm/test/CodeGen/DirectX/ContainerData/RuntimeInfoCS.ll
new file mode 100644
index 00000000000000..595e70092bb081
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/ContainerData/RuntimeInfoCS.ll
@@ -0,0 +1,41 @@
+; 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.psv0 = private constant [80 x i8] c"{{.*}}", section "PSV0", align 4
+
+define void @cs_main() #0 {
+entry:
+  ret void
+}
+
+attributes #0 = { "hlsl.numthreads"="8,8,1" "hlsl.shader"="compute" }
+
+!dx.valver = !{!0}
+
+!0 = !{i32 1, i32 7}
+
+; DXC: - Name:            PSV0
+; DXC-NEXT:   Size:            80
+; DXC-NEXT:    PSVInfo:
+; DXC-NEXT:      Version:         3
+; DXC-NEXT:      ShaderStage:     5
+; DXC-NEXT:      MinimumWaveLaneCount: 0
+; DXC-NEXT:      MaximumWaveLaneCount: 4294967295
+; DXC-NEXT:      UsesViewID:      0
+; DXC-NEXT:      SigInputVectors: 0
+; DXC-NEXT:      SigOutputVectors: [ 0, 0, 0, 0 ]
+; DXC-NEXT:      NumThreadsX:     8
+; DXC-NEXT:      NumThreadsY:     8
+; DXC-NEXT:      NumThreadsZ:     1
+; DXC-NEXT:      EntryName:       cs_main
+; DXC-NEXT:      ResourceStride:  24
+; DXC-NEXT:      Resources:       []
+; DXC-NEXT:      SigInputElements: []
+; DXC-NEXT:      SigOutputElements: []
+; DXC-NEXT:      SigPatchOrPrimElements: []
+; DXC-NEXT:      InputOutputMap:
+; DXC-NEXT:        - [  ]
+; DXC-NEXT:        - [  ]
+; DXC-NEXT:        - [  ]
+; DXC-NEXT:        - [  ]

github-actions · 2024-09-03T12:55:28Z

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

bharadwajy · 2024-09-03T16:12:26Z

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

+  dxil::ModuleMetadataInfo &MMI =
+      getAnalysis<DXILMetadataAnalysisWrapperPass>().getModuleMetadata();
+  assert(MMI.EntryPropertyVec.size() != 0 ||
+         TT.getEnvironment() == Triple::Library);


Can the shader stage from MMI viz., MMI.ShaderStage be used instead of once again getting the triple from module and getting the shader stage from the triple throughout this function?

Updated.

Would changes to the rest of the function to use MMI.ShaderStage as follows, also be appropriate?

diff --git a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp index c6998283850f..4b59251cfd90 100644 --- a/llvm/lib/Target/DirectX/DXContainerGlobals.cpp +++ b/llvm/lib/Target/DirectX/DXContainerGlobals.cpp @@ -145,7 +145,6 @@ void DXContainerGlobals::addPipelineStateValidationInfo( SmallString<256> Data; raw_svector_ostream OS(Data); PSVRuntimeInfo PSV; - Triple TT(M.getTargetTriple()); PSV.BaseData.MinimumWaveLaneCount = 0; PSV.BaseData.MaximumWaveLaneCount = std::numeric_limits<uint32_t>::max(); @@ -161,7 +160,7 @@ void DXContainerGlobals::addPipelineStateValidationInfo( // TODO: Lots more stuff to do here! // // See issue https://github.com/llvm/llvm-project/issues/96674. - switch (TT.getEnvironment()) { + switch (MMI.ShaderStage) { case Triple::Compute: PSV.BaseData.NumThreadsX = MMI.EntryPropertyVec[0].NumThreadsX; PSV.BaseData.NumThreadsY = MMI.EntryPropertyVec[0].NumThreadsY; @@ -171,10 +170,10 @@ void DXContainerGlobals::addPipelineStateValidationInfo( break; } - if (TT.getEnvironment() != Triple::Library) + if (MMI.ShaderStage != Triple::Library) PSV.EntryName = MMI.EntryPropertyVec[0].Entry->getName(); - PSV.finalize(TT.getEnvironment()); + PSV.finalize(MMI.ShaderStage); PSV.write(OS); Constant *Constant = ConstantDataArray::getString(M.getContext(), Data, /*AddNull*/ false);

Good idea.
Updated.

bharadwajy · 2024-09-03T19:28:55Z

llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp

@@ -103,6 +104,7 @@ class DXILTranslateMetadataLegacy : public ModulePass {
    AU.addRequired<DXILResourceWrapperPass>();
    AU.addRequired<DXILResourceMDWrapper>();
    AU.addRequired<ShaderFlagsAnalysisWrapper>();
+    AU.addRequired<DXILMetadataAnalysisWrapperPass>();


Does DXILMetadataAnalysisWrapperPass need to be added as required for DXILTranslateMetadata pass, given the changes in this PR do not consume results from DXILMetadataAnalysis pass?

The analysis needs to be run before preparePass which removes the attributes.
And we could consume result for translateMetadata pass as well.

bogner

This looks fine, but I think as we add more data to the metadata analysis the need to know its details in order to use its results will become untenable. It's probably reasonable to get this in as is and fix that in-tree though.

bogner · 2024-09-04T19:09:40Z

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

+  assert(MMI.EntryPropertyVec.size() != 0 ||
+         MMI.ShaderStage == Triple::Library);


What stops us from getting here with zero entry points? I guess we'll have diagnosed that and errored out earlier? Similarly, what if there are multiple entry points? Is that only possible for library shaders?

Good catch. Should be size == 1.
Multiple entry points or no entry point should be only possible for library shaders.

bogner · 2024-09-04T19:15:26Z

llvm/lib/Target/DirectX/DXContainerGlobals.cpp

+  switch (MMI.ShaderStage) {
+  case Triple::Compute:
+    PSV.BaseData.NumThreadsX = MMI.EntryPropertyVec[0].NumThreadsX;
+    PSV.BaseData.NumThreadsY = MMI.EntryPropertyVec[0].NumThreadsY;
+    PSV.BaseData.NumThreadsZ = MMI.EntryPropertyVec[0].NumThreadsZ;
+    break;
+  default:
+    break;
+  }
+
+  if (MMI.ShaderStage != Triple::Library)
+    PSV.EntryName = MMI.EntryPropertyVec[0].Entry->getName();


It seems pretty unfortunate that we need to know which fields are valid in the analysis result based on what stage it is - I see you made a similar comment on the metadata analysis PR, but I didn't notice it at the time. We should probably have a discussion on what the API of the metadata analysis should look like (cc @bharadwajy).

[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo

48cd044

Replace the hardcoded values for compute shader in DXContainer::addPipelineStateValidationInfo. Still missing wave size. Add preserved for previous passes so the information is not lost. Fix llvm/wg-hlsl#51

python3kgae added the backend:DirectX label Sep 3, 2024

clang-format.

a93e54e

bharadwajy reviewed Sep 3, 2024

View reviewed changes

python3kgae added 2 commits September 3, 2024 12:39

Use data from MMI.

aa93446

Replace local Triple with MMI.

816ceb4

bharadwajy reviewed Sep 3, 2024

View reviewed changes

bharadwajy approved these changes Sep 3, 2024

View reviewed changes

bogner approved these changes Sep 4, 2024

View reviewed changes

Fix assert condition

17d08e2

python3kgae merged commit eb2929d into llvm:main Sep 5, 2024
8 checks passed

python3kgae deleted the psv_runtime_info_cs branch September 5, 2024 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo #107101

[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo #107101

Uh oh!

python3kgae commented Sep 3, 2024

Uh oh!

llvmbot commented Sep 3, 2024

Uh oh!

github-actions bot commented Sep 3, 2024 •

edited

Loading

Uh oh!

bharadwajy Sep 3, 2024

Uh oh!

python3kgae Sep 3, 2024

Uh oh!

bharadwajy Sep 3, 2024

Uh oh!

python3kgae Sep 3, 2024

Uh oh!

bharadwajy Sep 3, 2024 •

edited

Loading

Uh oh!

python3kgae Sep 3, 2024

Uh oh!

bogner left a comment

Uh oh!

bogner Sep 4, 2024

Uh oh!

python3kgae Sep 4, 2024

Uh oh!

bogner Sep 4, 2024

Uh oh!

Uh oh!

Uh oh!

		assert(MMI.EntryPropertyVec.size() != 0 \|\|
		MMI.ShaderStage == Triple::Library);

[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo #107101

[DirectX] use DXILMetadataAnalysis to build PSVRuntimeInfo #107101

Uh oh!

Conversation

python3kgae commented Sep 3, 2024

Uh oh!

llvmbot commented Sep 3, 2024

Uh oh!

github-actions bot commented Sep 3, 2024 • 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!

bharadwajy Sep 3, 2024 • 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!

bogner left a comment

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!

github-actions bot commented Sep 3, 2024 •

edited

Loading

bharadwajy Sep 3, 2024 •

edited

Loading