ViewID tests and fix for constants in Phi nodes. (#310)

include/dxc/HLSL/ComputeViewIdState.h

@@ -14,6 +14,7 @@
 #pragma once
 #include "llvm/Pass.h"
 #include "dxc/HLSL/ControlDependence.h"
+#include "llvm/Support/GenericDomTree.h"
 
 #include <memory>
 #include <bitset>
@@ -29,6 +30,7 @@ namespace llvm {
   class Instruction;
   class ReturnInst;
   class Value;
+  class PHINode;
   class AnalysisUsage;
   class CallGraph;
   class CallGraphNode;
@@ -112,6 +114,7 @@ private:
   struct FuncInfo {
     FunctionReturnSet Returns;
     ControlDependence CtrlDep;
+    std::unique_ptr<llvm::DominatorTreeBase<llvm::BasicBlock> > pDomTree;
     void Clear();
   };
 
@@ -134,6 +137,9 @@ private:
   void CollectValuesContributingToOutputRec(EntryInfo &Entry,
                                             llvm::Value *pContributingValue,
                                             InstructionSetType &ContributingInstructions);
+  void CollectPhiCFValuesContributingToOutputRec(llvm::PHINode *pPhi,
+                                                 EntryInfo &Entry,
+                                                 InstructionSetType &ContributingInstructions);
   const ValueSetType &CollectReachingDecls(llvm::Value *pValue);
   void CollectReachingDeclsRec(llvm::Value *pValue, ValueSetType &ReachingDecls, ValueSetType &Visited);
   const ValueSetType &CollectStores(llvm::Value *pValue);

lib/HLSL/ComputeViewIdState.cpp

@@ -31,6 +31,8 @@ using namespace hlsl;
 using llvm::legacy::PassManager;
 using llvm::legacy::FunctionPassManager;
 using std::vector;
+using std::unordered_set;
+using std::unordered_map;
 
 #define DXILVIEWID_DBG   0
 
@@ -174,6 +176,7 @@ void DxilViewIdState::EntryInfo::Clear() {
 void DxilViewIdState::FuncInfo::Clear() {
   Returns.clear();
   CtrlDep.Clear();
+  pDomTree.reset();
 }
 
 void DxilViewIdState::DetermineMaxPackedLocation(DxilSignature &DxilSig, unsigned &MaxSigLoc) {
@@ -287,6 +290,13 @@ void DxilViewIdState::AnalyzeFunctions(EntryInfo &Entry) {
       }
     }
 
+    // Compute dominator relation.
+    pFuncInfo->pDomTree = make_unique<DominatorTreeBase<BasicBlock> >(false);
+    pFuncInfo->pDomTree->recalculate(*F);
+#if DXILVIEWID_DBG
+    pFuncInfo->pDomTree->print(dbgs());
+#endif
+
     // Compute postdominator relation.
     DominatorTreeBase<BasicBlock> PDR(true);
     PDR.recalculate(*F);
@@ -378,15 +388,7 @@ void DxilViewIdState::CollectValuesContributingToOutputRec(EntryInfo &Entry,
 
   // Handle special cases.
   if (PHINode *phi = dyn_cast<PHINode>(pContributingInst)) {
-    // For constant operands of phi, handle control dependence on incident edge.
-    unsigned iOp = 0;
-    for (auto it = phi->block_begin(), endIt = phi->block_end(); it != endIt; ++it, iOp++) {
-      Value *O = phi->getOperand(iOp);
-      if (isa<Constant>(O)) {
-        BasicBlock *pPredBB = *it;
-        CollectValuesContributingToOutputRec(Entry, pPredBB->getTerminator(), ContributingInstructions);
-      }
-    }
+    CollectPhiCFValuesContributingToOutputRec(phi, Entry, ContributingInstructions);
   } else if (isa<LoadInst>(pContributingInst) || 
              isa<AtomicCmpXchgInst>(pContributingInst) ||
              isa<AtomicRMWInst>(pContributingInst)) {
@@ -432,6 +434,93 @@ void DxilViewIdState::CollectValuesContributingToOutputRec(EntryInfo &Entry,
   }
 }
 
+// Only process control-dependent basic blocks for constant operands of the phi-function.
+// An obvious "definition" point for a constant operand is the predecessor along corresponding edge.
+// However, this may be too conservative and, as such, pick up extra control dependent BBs.
+// A better "definition" point is the highest dominator where it is still legal to "insert" constant assignment.
+// In this context, "legal" means that only one value "leaves" the dominator and reaches Phi.
+void DxilViewIdState::CollectPhiCFValuesContributingToOutputRec(PHINode *pPhi,
+                                                                EntryInfo &Entry,
+                                                                InstructionSetType &ContributingInstructions) {
+  Function *F = pPhi->getParent()->getParent();
+  FuncInfo *pFuncInfo = m_FuncInfo[F].get();
+  unordered_map<DomTreeNodeBase<BasicBlock> *, Value *> DomTreeMarkers;
+
+  // Mark predecessors of each value, so that there is a legal "definition" point.
+  for (unsigned i = 0; i < pPhi->getNumOperands(); i++) {
+    Value *pValue = pPhi->getIncomingValue(i);
+    BasicBlock *pBB = pPhi->getIncomingBlock(i);
+    DomTreeNodeBase<BasicBlock> *pDomNode = pFuncInfo->pDomTree->getNode(pBB);
+    auto it = DomTreeMarkers.emplace(pDomNode, pValue);
+    DXASSERT_NOMSG(it.second || it.first->second == pValue); it;
+  }
+  // Mark the dominator tree with "definition" values, walking up to the parent.
+  for (unsigned i = 0; i < pPhi->getNumOperands(); i++) {
+    Value *pValue = pPhi->getIncomingValue(i);
+    BasicBlock *pDefBB = &F->getEntryBlock();
+    if (Instruction *pDefInst = dyn_cast<Instruction>(pValue)) {
+      pDefBB = pDefInst->getParent();
+    }
+
+    BasicBlock *pBB = pPhi->getIncomingBlock(i);
+    if (pBB == pDefBB) {
+      continue; // we already handled the predecessor.
+    }
+    DomTreeNodeBase<BasicBlock> *pDomNode = pFuncInfo->pDomTree->getNode(pBB);
+    pDomNode = pDomNode->getIDom();
+    while (pDomNode) {
+      auto it = DomTreeMarkers.emplace(pDomNode, pValue);
+      if (!it.second) {
+        if (it.first->second != pValue && it.first->second != nullptr) {
+          if (!isa<Constant>(it.first->second) || !isa<Constant>(pValue)) {
+            // Unless both are different constants, mark the "definition" point as illegal.
+            it.first->second = nullptr;
+            // If both are constants, leave the marker of the first one.
+          }
+        }
+        break;
+      }
+
+      // Do not go higher than a legal definition point.
+      pBB = pDomNode->getBlock();
+      if (pBB == pDefBB)
+        break;
+
+      pDomNode = pDomNode->getIDom();
+    }
+  }
+
+  // Handle control dependence for Constant arguments of Phi.
+  for (unsigned i = 0; i < pPhi->getNumOperands(); i++) {
+    Value *pValue = pPhi->getIncomingValue(i);
+    if (!isa<Constant>(pValue))
+      continue;
+
+    // Determine the higher legal "definition" point.
+    BasicBlock *pBB = pPhi->getIncomingBlock(i);
+    DomTreeNodeBase<BasicBlock> *pDomNode = pFuncInfo->pDomTree->getNode(pBB);
+    DomTreeNodeBase<BasicBlock> *pDefDomNode = pDomNode;
+    while (pDomNode) {
+      auto it = DomTreeMarkers.find(pDomNode);
+      DXASSERT_NOMSG(it != DomTreeMarkers.end());
+      if (it->second != pValue) {
+        DXASSERT_NOMSG(it->second == nullptr || isa<Constant>(it->second));
+        break;
+      }
+
+      pDefDomNode = pDomNode;
+      pDomNode = pDomNode->getIDom();
+    }
+
+    // Handle control dependence of this constant argument highest legal "definition" point.
+    pBB = pDefDomNode->getBlock();
+    const BasicBlockSet &CtrlDepSet = pFuncInfo->CtrlDep.GetCDBlocks(pBB);
+    for (BasicBlock *B : CtrlDepSet) {
+      CollectValuesContributingToOutputRec(Entry, B->getTerminator(), ContributingInstructions);
+    }
+  }
+}
+
 const DxilViewIdState::ValueSetType &DxilViewIdState::CollectReachingDecls(Value *pValue) {
   auto it = m_ReachingDeclsCache.emplace(pValue, ValueSetType());
   if (it.second) {

tools/clang/test/CodeGenHLSL/viewid/viewid01.hlsl

@@ -0,0 +1,16 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Number of inputs: 4, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 1 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0 }
+// CHECK:   output 1 depends on inputs: { 1, 2 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 3 }
+
+float4 main(float4 a : AAA, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a;
+  ret.y += a.z + viewid;
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid02.hlsl

@@ -0,0 +1,17 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Number of inputs: 8, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 0, 3 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 4, 7 }
+// CHECK:   output 1 depends on inputs: { 1 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 0, 1, 2, 3, 4, 5, 6, 7 }
+
+float4 main(float4 a : AAA, float4 b : BBB, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a;
+  ret.x = a.x + viewid + b.x * b.w;
+  ret.w += dot(a, b) * viewid;
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid03.hlsl

@@ -0,0 +1,20 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Number of inputs: 8, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 0 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 5, 7 }
+// CHECK:   output 1 depends on inputs: { 1 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 3 }
+
+float4 main(float4 a : AAA, uint4 b : BBB, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a;
+  ret.x = a.x + viewid + b.y;
+  [branch]
+  if (b.w > 3) {
+    ret.x = 0;
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid04.hlsl

@@ -0,0 +1,20 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Number of inputs: 8, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 0 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 1, 5, 6, 7 }
+// CHECK:   output 1 depends on inputs: { 1 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 3 }
+
+float4 main(float4 a : AAA, uint4 b : BBB, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a;
+  ret.x = a.x + viewid + b.y;
+  [loop]
+  for (int i = b.z; i < b.w; i++) {
+    ret.x += a.y;
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid05.hlsl

@@ -0,0 +1,20 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Number of inputs: 12, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 0 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 5, 11 }
+// CHECK:   output 1 depends on inputs: { 1 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 3 }
+
+float4 main(float4 a : AAA, uint4 b[2] : BBB, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a;
+  ret.x = a.x + viewid + b[0].y;
+  [branch]
+  if (b[1].w > 3) {
+    ret.x = 0;
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid06.hlsl

@@ -0,0 +1,27 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Name                 Index             InterpMode DynIdx
+// CHECK: -------------------- ----- ---------------------- ------
+// CHECK: AAA                      0                 linear
+// CHECK: BBB                      0        nointerpolation     yw
+
+// CHECK: Number of inputs: 12, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 0 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 5, 7, 9, 11 }
+// CHECK:   output 1 depends on inputs: { 1 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 3 }
+
+uint g1;
+
+float4 main(float4 a : AAA, uint4 b[2] : BBB, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a;
+  ret.x = a.x + viewid + b[g1].y;
+  [branch]
+  if (b[g1].w > 3) {
+    ret.x = 0;
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid07.hlsl

@@ -0,0 +1,20 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Number of inputs: 4, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: { 2 }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 1 }
+// CHECK:   output 1 depends on inputs: { 1 }
+// CHECK:   output 2 depends on inputs: { 0, 1, 2, 3 }
+// CHECK:   output 3 depends on inputs: { 1 }
+
+uint g1;
+
+float4 main(float4 a : AAA, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a.y;
+  float x[2] = {a.x, a.z};
+  x[a.w] = viewid;
+  ret.z += x[g1];
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid08.hlsl

@@ -0,0 +1,28 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Name                 Index             InterpMode DynIdx
+// CHECK: -------------------- ----- ---------------------- ------
+// CHECK: AAA                      0                 linear      y
+
+// CHECK: Number of inputs: 12, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: {  }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 1, 2, 3, 5, 8, 9 }
+// CHECK:   output 1 depends on inputs: { 2 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 2 }
+
+uint g1;
+
+float4 main(float4 a[3] : AAA, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a[0].z;
+  if (a[g1].y) {
+    if (g1) {
+      if (a[0].w) {
+        ret.x = a[2].x;
+      }
+    }
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid09.hlsl

@@ -0,0 +1,28 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Name                 Index             InterpMode DynIdx
+// CHECK: -------------------- ----- ---------------------- ------
+// CHECK: AAA                      0                 linear      y
+
+// CHECK: Number of inputs: 12, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: {  }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 1, 2, 3, 5, 6, 8, 9 }
+// CHECK:   output 1 depends on inputs: { 2 }
+// CHECK:   output 2 depends on inputs: { 2 }
+// CHECK:   output 3 depends on inputs: { 2 }
+
+uint g1;
+
+float4 main(float4 a[3] : AAA, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = a[0].z;
+  if (a[g1].y) {
+    for (uint i = g1; i <= a[1].z; i++) {
+      if (a[0].w) {
+        ret.x = a[2].x;
+      }
+    }
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid10.hlsl

@@ -0,0 +1,31 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Name                 Index             InterpMode DynIdx
+// CHECK: -------------------- ----- ---------------------- ------
+// CHECK: AAA                      0                 linear      y
+
+// CHECK: Number of inputs: 8, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: {  }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 1, 5 }
+// CHECK:   output 3 depends on inputs: { 1, 3, 5 }
+
+uint g1;
+
+float4 main(float4 a[2] : AAA, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = 2;
+  [branch]
+  switch (a[g1].y) {
+  case 0:
+    [branch]
+    if (a[0].x) {
+      ret.x += 5;
+    }
+    break;
+  case 1:
+    ret.w += a[0].w;
+    break;
+  }
+  return ret;
+}

tools/clang/test/CodeGenHLSL/viewid/viewid11.hlsl

@@ -0,0 +1,38 @@
+// RUN: %dxc -E main -T ps_6_1 %s | FileCheck %s
+
+// CHECK: Name                 Index             InterpMode DynIdx
+// CHECK: -------------------- ----- ---------------------- ------
+// CHECK: AAA                      0                 linear     yz
+
+// CHECK: Number of inputs: 12, outputs: 4, patchconst: 0
+// CHECK: Outputs dependent on ViewId: {  }
+// CHECK: Inputs contributing to computation of Outputs:
+// CHECK:   output 0 depends on inputs: { 0, 1, 5, 7, 9 }
+// CHECK:   output 3 depends on inputs: { 1, 2, 5, 6, 9, 10 }
+
+uint g1;
+
+float4 main(float4 a[3] : AAA, uint viewid : SV_ViewId) : SV_Target
+{
+  float4 ret = 2;
+  [branch]
+  switch (a[g1].y) {
+  case 0:
+    [branch]
+    if (a[0].x) {
+      ret.x += a[1].w;
+    }
+    break;
+  case 1:
+  case 2:
+    ret.w += 777;
+    break;
+  case 3:
+  case 77:
+    break;
+  default:
+    ret.w = a[g1].z;
+    break;
+  }
+  return ret;
+}

tools/clang/unittests/HLSL/CompilerTest.cpp

@@ -944,6 +944,7 @@ public:
   TEST_METHOD(DxilGen_StoreOutput)
   TEST_METHOD(ConstantFolding)
   TEST_METHOD(HoistConstantArray)
+  TEST_METHOD(ViewID)
 
   dxc::DxcDllSupport m_dllSupport;
   VersionSupportInfo m_ver;
@@ -4617,3 +4618,17 @@ TEST_F(CompilerTest, WhenSigMismatchPCFunctionThenFail) {
   VERIFY_ARE_NOT_EQUAL(string::npos, failLog.find(
     "Signature element SV_Position, referred to by patch constant function, is not found in corresponding hull shader output."));
 }
+
+TEST_F(CompilerTest, ViewID) {
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid01.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid02.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid03.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid04.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid05.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid06.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid07.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid08.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid09.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid10.hlsl");
+  CodeGenTestCheck(L"..\\CodeGenHLSL\\viewid\\viewid11.hlsl");
+}