PIX: Emit correct input sig elements and view ID state into PSV0 when input sigs are changed by debug instrumentation (#6351)

Two intertwined issues:

1:
The shader-debug-instrumentation and pixel-hit-counter passes add
SV_VertexId+SV_InstanceId to the input sig for a VS if they are not
present, and SV_Position for the PS. The changes in PixPassHelpers.cpp
centralize the SV_Position case and fix up a few missing fields whose
absence was breaking PIX shader debugging on WARP. The calller now has
to pass the upstream shader's SV_Position row, if it exists. (PIX has
been updated to do this.)

2:
However, independent of these tweaks, adding these new values means that
the view id metadata would be incorrect. A long-standing assert would
fire, but is herein fixed, since the above work exacerbated this
problem. The assert in question is this one in
CopyViewIDStateForOutputToPSV that previously fired when a
debug-instrumented shader was subsequently wrapped into a container:
```     DXASSERT_NOMSG((InputScalars <= IOTable.InputVectors * 4) &&
                   (IOTable.InputVectors * 4 - InputScalars < 4)); 
```
(InputScalars, which comes from the serialized state in PSV0, was too
small, since it did not include the added system values.)

To fix up these data, the caller now has to invoke the
generate-view-id-state pass, and follow that with the emit-metadata
pass, since emitting the metadata in the debug pass itself is now too
early (and that call has been deleted). (PIX has been updated to do
invoke these passes.)

These changes together are sufficient to allow PIX shader debugging to
operate on WARP in those cases where these system-values were not
included by the application, with correct view-id metadata, and the
assert is happy.

Author: jeffnn

lib/DxilPIXPasses/DxilAddPixelHitInstrumentation.cpp

@@ -32,22 +32,25 @@ class DxilAddPixelHitInstrumentation : public ModulePass {
   bool AddPixelCost = false;
   int RTWidth = 1024;
   int NumPixels = 128;
-  int SVPositionIndex = -1;
 
 public:
   static char ID; // Pass identification, replacement for typeid
   explicit DxilAddPixelHitInstrumentation() : ModulePass(ID) {}
-  StringRef getPassName() const override { return "DXIL Constant Color Mod"; }
+  StringRef getPassName() const override {
+    return "DXIL Add Pixel Hit Instrumentation";
+  }
   void applyOptions(PassOptions O) override;
   bool runOnModule(Module &M) override;
+  unsigned m_upstreamSVPositionRow;
 };
 
 void DxilAddPixelHitInstrumentation::applyOptions(PassOptions O) {
   GetPassOptionBool(O, "force-early-z", &ForceEarlyZ, false);
   GetPassOptionBool(O, "add-pixel-cost", &AddPixelCost, false);
   GetPassOptionInt(O, "rt-width", &RTWidth, 0);
   GetPassOptionInt(O, "num-pixels", &NumPixels, 0);
-  GetPassOptionInt(O, "sv-position-index", &SVPositionIndex, 0);
+  GetPassOptionUnsigned(O, "upstream-sv-position-row", &m_upstreamSVPositionRow,
+                        0);
 }
 
 bool DxilAddPixelHitInstrumentation::runOnModule(Module &M) {
@@ -63,37 +66,8 @@ bool DxilAddPixelHitInstrumentation::runOnModule(Module &M) {
     DM.m_ShaderFlags.SetForceEarlyDepthStencil(true);
   }
 
-  hlsl::DxilSignature &InputSignature = DM.GetInputSignature();
-
-  auto &InputElements = InputSignature.GetElements();
-
-  unsigned SV_Position_ID;
-
-  auto SV_Position =
-      std::find_if(InputElements.begin(), InputElements.end(),
-                   [](const std::unique_ptr<DxilSignatureElement> &Element) {
-                     return Element->GetSemantic()->GetKind() ==
-                            hlsl::DXIL::SemanticKind::Position;
-                   });
-
-  // SV_Position, if present, has to have full mask, so we needn't worry
-  // about the shader having selected components that don't include x or y.
-  // If not present, we add it.
-  if (SV_Position == InputElements.end()) {
-    auto SVPosition =
-        llvm::make_unique<DxilSignatureElement>(DXIL::SigPointKind::PSIn);
-    SVPosition->Initialize("Position", hlsl::CompType::getF32(),
-                           hlsl::DXIL::InterpolationMode::Linear, 1, 4,
-                           SVPositionIndex == -1 ? 0 : SVPositionIndex, 0);
-    SVPosition->AppendSemanticIndex(0);
-    SVPosition->SetSigPointKind(DXIL::SigPointKind::PSIn);
-    SVPosition->SetKind(hlsl::DXIL::SemanticKind::Position);
-
-    auto index = InputSignature.AppendElement(std::move(SVPosition));
-    SV_Position_ID = InputElements[index]->GetID();
-  } else {
-    SV_Position_ID = SV_Position->get()->GetID();
-  }
+  auto SV_Position_ID =
+      PIXPassHelpers::FindOrAddSV_Position(DM, m_upstreamSVPositionRow);
 
   auto EntryPointFunction = PIXPassHelpers::GetEntryFunction(DM);
 

lib/DxilPIXPasses/DxilDebugInstrumentation.cpp

@@ -19,6 +19,7 @@
 #include "dxc/DxilPIXPasses/DxilPIXPasses.h"
 #include "dxc/DxilPIXPasses/DxilPIXVirtualRegisters.h"
 #include "dxc/HLSL/DxilGenerationPass.h"
+#include "dxc/Support/Global.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/IR/Constants.h"
@@ -282,6 +283,8 @@ class DxilDebugInstrumentation : public ModulePass {
   unsigned m_LastInstruction = static_cast<unsigned>(-1);
 
   uint64_t m_UAVSize = 1024 * 1024;
+  unsigned m_upstreamSVPositionRow;
+
   struct PerFunctionValues {
     CallInst *UAVHandle = nullptr;
     Instruction *CounterOffset = nullptr;
@@ -378,17 +381,33 @@ void DxilDebugInstrumentation::applyOptions(PassOptions O) {
   GetPassOptionUnsigned(O, "parameter1", &m_Parameters.Parameters[1], 0);
   GetPassOptionUnsigned(O, "parameter2", &m_Parameters.Parameters[2], 0);
   GetPassOptionUInt64(O, "UAVSize", &m_UAVSize, 1024 * 1024);
+  GetPassOptionUnsigned(O, "upstreamSVPositionRow", &m_upstreamSVPositionRow,
+                        0);
 }
 
 uint32_t DxilDebugInstrumentation::UAVDumpingGroundOffset() {
   return static_cast<uint32_t>(m_UAVSize / 2);
 }
 
-static unsigned FindOrAddInputSignatureElement(
-    hlsl::DxilSignature &InputSignature, const char *name,
-    DXIL::SigPointKind sigPointKind, hlsl::DXIL::SemanticKind semanticKind) {
+unsigned int GetNextEmptyRow(
+    std::vector<std::unique_ptr<DxilSignatureElement>> const &Elements) {
+  unsigned int Row = 0;
+  for (auto const &Element : Elements) {
+    Row = std::max<unsigned>(Row, Element->GetStartRow() + Element->GetRows());
+  }
+  return Row;
+}
 
-  auto &InputElements = InputSignature.GetElements();
+unsigned FindOrAddVSInSignatureElementForInstanceOrVertexID(
+    hlsl::DxilSignature &InputSignature,
+    hlsl::DXIL::SemanticKind semanticKind) {
+  DXASSERT(InputSignature.GetSigPointKind() == DXIL::SigPointKind::VSIn,
+           "Unexpected SigPointKind in input signature");
+  DXASSERT(semanticKind == DXIL::SemanticKind::InstanceID ||
+               semanticKind == DXIL::SemanticKind::VertexID,
+           "This function only expects InstaceID or VertexID");
+
+  auto const &InputElements = InputSignature.GetElements();
 
   auto ExistingElement =
       std::find_if(InputElements.begin(), InputElements.end(),
@@ -397,13 +416,16 @@ static unsigned FindOrAddInputSignatureElement(
                    });
 
   if (ExistingElement == InputElements.end()) {
-    auto AddedElement = llvm::make_unique<DxilSignatureElement>(sigPointKind);
-    AddedElement->Initialize(name, hlsl::CompType::getF32(),
-                             hlsl::DXIL::InterpolationMode::Undefined, 1, 1);
+    auto AddedElement =
+        llvm::make_unique<DxilSignatureElement>(DXIL::SigPointKind::VSIn);
+    unsigned Row = GetNextEmptyRow(InputElements);
+    AddedElement->Initialize(
+        hlsl::Semantic::Get(semanticKind)->GetName(), hlsl::CompType::getU32(),
+        hlsl::DXIL::InterpolationMode::Constant, 1, 1, Row, 0);
     AddedElement->AppendSemanticIndex(0);
-    AddedElement->SetSigPointKind(sigPointKind);
     AddedElement->SetKind(semanticKind);
-
+    AddedElement->SetUsageMask(1);
+    // AppendElement sets the element's ID by default
     auto index = InputSignature.AppendElement(std::move(AddedElement));
     return InputElements[index]->GetID();
   } else {
@@ -429,12 +451,12 @@ DxilDebugInstrumentation::addRequiredSystemValues(BuilderContext &BC,
     break;
   case DXIL::ShaderKind::Vertex: {
     hlsl::DxilSignature &InputSignature = BC.DM.GetInputSignature();
-    SVIndices.VertexShader.VertexId = FindOrAddInputSignatureElement(
-        InputSignature, "VertexId", DXIL::SigPointKind::VSIn,
-        hlsl::DXIL::SemanticKind::VertexID);
-    SVIndices.VertexShader.InstanceId = FindOrAddInputSignatureElement(
-        InputSignature, "InstanceId", DXIL::SigPointKind::VSIn,
-        hlsl::DXIL::SemanticKind::InstanceID);
+    SVIndices.VertexShader.VertexId =
+        FindOrAddVSInSignatureElementForInstanceOrVertexID(
+            InputSignature, hlsl::DXIL::SemanticKind::VertexID);
+    SVIndices.VertexShader.InstanceId =
+        FindOrAddVSInSignatureElementForInstanceOrVertexID(
+            InputSignature, hlsl::DXIL::SemanticKind::InstanceID);
   } break;
   case DXIL::ShaderKind::Geometry:
   case DXIL::ShaderKind::Hull:
@@ -443,34 +465,8 @@ DxilDebugInstrumentation::addRequiredSystemValues(BuilderContext &BC,
     // in the input signature
     break;
   case DXIL::ShaderKind::Pixel: {
-    hlsl::DxilSignature &InputSignature = BC.DM.GetInputSignature();
-    auto &InputElements = InputSignature.GetElements();
-
-    auto Existing_SV_Position =
-        std::find_if(InputElements.begin(), InputElements.end(),
-                     [](const std::unique_ptr<DxilSignatureElement> &Element) {
-                       return Element->GetSemantic()->GetKind() ==
-                              hlsl::DXIL::SemanticKind::Position;
-                     });
-
-    // SV_Position, if present, has to have full mask, so we needn't worry
-    // about the shader having selected components that don't include x or y.
-    // If not present, we add it.
-    if (Existing_SV_Position == InputElements.end()) {
-      auto Added_SV_Position =
-          llvm::make_unique<DxilSignatureElement>(DXIL::SigPointKind::PSIn);
-      Added_SV_Position->Initialize("Position", hlsl::CompType::getF32(),
-                                    hlsl::DXIL::InterpolationMode::Linear, 1,
-                                    4);
-      Added_SV_Position->AppendSemanticIndex(0);
-      Added_SV_Position->SetSigPointKind(DXIL::SigPointKind::PSIn);
-      Added_SV_Position->SetKind(hlsl::DXIL::SemanticKind::Position);
-
-      auto index = InputSignature.AppendElement(std::move(Added_SV_Position));
-      SVIndices.PixelShader.Position = InputElements[index]->GetID();
-    } else {
-      SVIndices.PixelShader.Position = Existing_SV_Position->get()->GetID();
-    }
+    SVIndices.PixelShader.Position =
+        PIXPassHelpers::FindOrAddSV_Position(BC.DM, m_upstreamSVPositionRow);
   } break;
   default:
     assert(false); // guaranteed by runOnModule
@@ -1515,8 +1511,6 @@ bool DxilDebugInstrumentation::RunOnFunction(Module &M, DxilModule &DM,
     }
   }
 
-  DM.ReEmitDxilResources();
-
   return true;
 }
 

lib/DxilPIXPasses/PixPassHelpers.cpp

@@ -450,6 +450,41 @@ void ReplaceAllUsesOfInstructionWithNewValueAndDeleteInstruction(
   delete Instr;
 }
 
+unsigned int FindOrAddSV_Position(hlsl::DxilModule &DM,
+                                  unsigned UpStreamSVPosRow) {
+  hlsl::DxilSignature &InputSignature = DM.GetInputSignature();
+  auto &InputElements = InputSignature.GetElements();
+
+  auto Existing_SV_Position =
+      std::find_if(InputElements.begin(), InputElements.end(),
+                   [](const std::unique_ptr<DxilSignatureElement> &Element) {
+                     return Element->GetSemantic()->GetKind() ==
+                            hlsl::DXIL::SemanticKind::Position;
+                   });
+
+  // SV_Position, if present, has to have full mask, so we needn't worry
+  // about the shader having selected components that don't include x or y.
+  // If not present, we add it.
+  if (Existing_SV_Position == InputElements.end()) {
+    unsigned int StartColumn = 0;
+    unsigned int RowCount = 1;
+    unsigned int ColumnCount = 4;
+    auto Added_SV_Position =
+        llvm::make_unique<DxilSignatureElement>(DXIL::SigPointKind::PSIn);
+    Added_SV_Position->Initialize("Position", hlsl::CompType::getF32(),
+                                  hlsl::DXIL::InterpolationMode::Linear,
+                                  RowCount, ColumnCount, UpStreamSVPosRow,
+                                  StartColumn);
+    Added_SV_Position->AppendSemanticIndex(0);
+    Added_SV_Position->SetKind(hlsl::DXIL::SemanticKind::Position);
+    // AppendElement sets the element's ID by default
+    auto index = InputSignature.AppendElement(std::move(Added_SV_Position));
+    return InputElements[index]->GetID();
+  } else {
+    return Existing_SV_Position->get()->GetID();
+  }
+}
+
 #ifdef PIX_DEBUG_DUMP_HELPER
 
 static int g_logIndent = 0;

lib/DxilPIXPasses/PixPassHelpers.h

@@ -74,4 +74,6 @@ ExpandedStruct ExpandStructType(llvm::LLVMContext &Ctx,
                                 llvm::Type *OriginalPayloadStructType);
 void ReplaceAllUsesOfInstructionWithNewValueAndDeleteInstruction(
     llvm::Instruction *Instr, llvm::Value *newValue, llvm::Type *newType);
+unsigned int FindOrAddSV_Position(hlsl::DxilModule &DM,
+                                  unsigned UpStreamSVPosRow);
 } // namespace PIXPassHelpers

tools/clang/test/HLSLFileCheck/pix/DebugBasic.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxc -Emain -Tps_6_0 %s | %opt -S -hlsl-dxil-debug-instrumentation,UAVSize=128 | %FileCheck %s
+// RUN: %dxc -Emain -Tps_6_0 %s | %opt -S -hlsl-dxil-debug-instrumentation,UAVSize=128,upstreamSVPositionRow=2 -hlsl-dxilemit | %FileCheck %s
 
 // Check that the basic starting header is present:
 
@@ -22,7 +22,12 @@
 // CHECK: and i32 
 // CHECK: or i32
 
-
+// Check that the correct metadata was emitted for the requested SV_Position at row 2, col 0, 1 row, 4 cols.
+// See DxilMDHelper::EmitSignatureElement for the meaning of these entries:
+//             ID                 TypeF32 SemKin Sem-Idx-Vec           interp  Rows Cols   Row    Col
+//              |                     |     |       |                    |      |     |      |     |
+// CHECK: !{i32 0, !"SV_Position", i8 9, i8 3, ![[SEMIDXVEC:[0-9]*]], i8 2, i32 1, i8 4, i32 2, i8 0, null}
+// CHECK: ![[SEMIDXVEC]] = !{i32 0}
 
 [RootSignature("")]
 float4 main() : SV_Target {

tools/clang/test/HLSLFileCheck/pix/DebugEmitCorrectViewIdState.hlsl

@@ -0,0 +1,13 @@
+// RUN: %dxc -Emain -Tvs_6_0 %s | %opt -S -hlsl-dxil-debug-instrumentation,parameter0=1,parameter1=2 -viewid-state -hlsl-dxilemit | %FileCheck %s
+
+// CHECK: !dx.viewIdState = !{![[VIEWIDDATA:[0-9]*]]}
+
+// The debug instrumentation will have added SV_InstanceId and SV_VertexId to the input signature for this VS.
+// If view id state is correct, then this entry should have expanded to 7 i32s (previously it would have been 2)
+// CHECK: ![[VIEWIDDATA]] = !{[7 x i32]
+
+
+[RootSignature("")]
+float4 main() : SV_Position{
+  return float4(0,0,0,0);
+}

tools/clang/test/HLSLFileCheck/pix/DebugEmitCorrectViewIdStatePS.hlsl

@@ -0,0 +1,16 @@
+// RUN: %dxc -Emain -Tps_6_0 %s | %opt -S -hlsl-dxil-debug-instrumentation,parameter0=1,parameter1=2 -viewid-state -hlsl-dxilemit | %FileCheck %s
+
+// CHECK: !dx.viewIdState = !{![[VIEWIDDATA:[0-9]*]]}
+
+// The debug instrumentation will have added SV_Position to the input signature for this PS.
+// If view id state is correct, then this entry should have expanded to 6 i32s (previously it would have been 4)
+// CHECK: ![[VIEWIDDATA]] = !{[6 x i32]
+
+
+struct VS_OUTPUT {
+  float2 Tex : TEXCOORD0;
+};
+
+float4 main(VS_OUTPUT input) : SV_Target {
+  return input.Tex.xyxy;
+}

tools/clang/test/HLSLFileCheck/pix/DebugVSParameters.hlsl

@@ -1,4 +1,4 @@
-// RUN: %dxc -Emain -Tvs_6_0 %s | %opt -S -hlsl-dxil-debug-instrumentation,parameter0=1,parameter1=2 | %FileCheck %s
+// RUN: %dxc -Emain -Tvs_6_0 %s | %opt -S -hlsl-dxil-debug-instrumentation,parameter0=1,parameter1=2 -hlsl-dxilemit | %FileCheck %s
 
 // Check that the instance and vertex id are parsed and present:
 
@@ -9,7 +9,15 @@
 // CHECK: %CompareToInstanceId = icmp eq i32 %InstanceId, 2
 // CHECK: %CompareBoth = and i1 %CompareToVertId, %CompareToInstanceId
 
-
+// Check that the correct metadata was emitted for vertex id and instance id. 
+// They should have 1 row, 1 column each. Vertex ID first at row 0, then instnce at row 1.
+// (With each row have the same value as the corresponding ID)
+// See DxilMDHelper::EmitSignatureElement for the meaning of these entries:
+//               ID                   TypeU32 SemKin Sem-Idx         interp  Rows  Cols   Row   Col
+//                |                     |     |       |                |      |     |      |     |
+// CHECK: = !{i32 0, !"SV_VertexID", i8 5, i8 1, ![[VIDID:[0-9]*]], i8 1, i32 1, i8 1, i32 0, i8 0,
+//                |                       |     |     |                |      |     |      |     |
+// CHECK: = !{i32 1, !"SV_InstanceID", i8 5, i8 2, ![[IID:[0-9]*]], i8 1, i32 1, i8 1, i32 1, i8 0,
 [RootSignature("")]
 float4 main() : SV_Position{
   return float4(0,0,0,0);