Merged PR 97: Remove phi on resources, follow phi and select on handles in validation

lib/HLSL/DxilCondenseResources.cpp

@@ -502,12 +502,12 @@ public:
 
     bChanged |= AllocateDxilResources(DM);
 
+    // Make sure no select on resource.
+    bChanged |= RemovePhiOnResource();
+
     if (m_bIsLib)
       return bChanged;
 
-    // Make sure no select on resource.
-    RemovePhiOnResource();
-
     bChanged = true;
 
     // Load up debug information, to cross-reference values and the instructions
@@ -530,7 +530,7 @@ public:
   }
 
 private:
-  void RemovePhiOnResource();
+  bool RemovePhiOnResource();
   void UpdateResourceSymbols();
   void TranslateDxilResourceUses(DxilResourceBase &res);
   void GenerateDxilResourceHandles();
@@ -622,7 +622,7 @@ void UpdateOperandSelect(Instruction *SelInst, Instruction *Prototype,
   }
 }
 
-void RemovePhiOnResourceImp(Function *F, hlsl::OP *hlslOP) {
+bool RemovePhiOnResourceImp(Function *F, hlsl::OP *hlslOP) {
   Value *opArg = hlslOP->GetU32Const(
       (unsigned)DXIL::OpCode::CreateHandleForLib);
 
@@ -676,21 +676,25 @@ void RemovePhiOnResourceImp(Function *F, hlsl::OP *hlslOP) {
       SelInst->replaceAllUsesWith(UndefValue::get(SelInst->getType()));
       SelInst->eraseFromParent();
     }
+    return true;
   }
+  return false;
 }
 } // namespace
 
-void DxilLowerCreateHandleForLib::RemovePhiOnResource() {
+bool DxilLowerCreateHandleForLib::RemovePhiOnResource() {
+  bool bChanged = false;
   hlsl::OP *hlslOP = m_DM->GetOP();
   for (Function &F : m_DM->GetModule()->functions()) {
     if (hlslOP->IsDxilOpFunc(&F)) {
       hlsl::OP::OpCodeClass opClass;
       if (hlslOP->GetOpCodeClass(&F, opClass) &&
           opClass == DXIL::OpCodeClass::CreateHandleForLib) {
-        RemovePhiOnResourceImp(&F, hlslOP);
+        bChanged |= RemovePhiOnResourceImp(&F, hlslOP);
       }
     }
   }
+  return bChanged;
 }
 
 // LegacyLayout.

lib/HLSL/DxilValidation.cpp

@@ -832,40 +832,55 @@ ValidateSignatureAccess(Instruction *I, DxilSignature &sig, Value *sigID,
   return &SE;
 }
 
+static CallInst *GetHandleFromValue(Value *V, DenseSet<Value *> &Checked);
 static CallInst *GetHandleFromPhi(PHINode *Phi, DenseSet<Value *> &Checked) {
   // TODO: validate all incoming values for phi is same kind and class.
   for (Value *V : Phi->incoming_values()) {
-    if (Checked.count(V))
-      continue;
+    if (CallInst *CI = GetHandleFromValue(V, Checked))
+      return CI;
+  }
+  return nullptr;
+}
+static CallInst *GetHandleFromSelect(SelectInst *Sel, DenseSet<Value *> &Checked) {
+  // TODO: validate all incoming values for select is same kind and class.
+  for (Value *V = Sel->getTrueValue(), *F = Sel->getFalseValue(); V != F; V = F) {
+    if (CallInst *CI = GetHandleFromValue(V, Checked))
+      return CI;
+  }
+  return nullptr;
+}
+static CallInst *GetHandleFromValue(Value *V, DenseSet<Value *> &Checked) {
+  if (!Checked.count(V)) {
     Checked.insert(V);
     if (CallInst *CI = dyn_cast<CallInst>(V)) {
       return CI;
     }
-    if (PHINode *P = dyn_cast<PHINode>(V)) {
+    else if (PHINode *P = dyn_cast<PHINode>(V)) {
       if (CallInst *CI = GetHandleFromPhi(P, Checked)) {
         return CI;
       }
     }
+    else if (SelectInst *S = dyn_cast<SelectInst>(V)) {
+      if (CallInst *CI = GetHandleFromSelect(S, Checked)) {
+        return CI;
+      }
+    }
   }
   return nullptr;
 }
 
 static DXIL::SamplerKind GetSamplerKind(Value *samplerHandle,
                                         ValidationContext &ValCtx) {
-  if (PHINode *Phi = dyn_cast<PHINode>(samplerHandle)) {
+  if (!isa<CallInst>(samplerHandle)) {
     DenseSet<Value *> Checked;
-    samplerHandle = GetHandleFromPhi(Phi, Checked);
-    if (!samplerHandle) {
+    if (CallInst *CI = GetHandleFromValue(samplerHandle, Checked)) {
+      samplerHandle = CI;
+    } else {
       ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
       return DXIL::SamplerKind::Invalid;
     }
   }
 
-  if (!isa<CallInst>(samplerHandle)) {
-    ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
-    return DXIL::SamplerKind::Invalid;
-  }
-
   DxilInst_CreateHandle createHandle(cast<CallInst>(samplerHandle));
   if (!createHandle) {
     auto EmitError = [&]() -> DXIL::SamplerKind {
@@ -947,19 +962,16 @@ static DXIL::ResourceKind GetResourceKindAndCompTy(Value *handle, DXIL::Componen
     ValidationContext &ValCtx) {
   CompTy = DXIL::ComponentType::Invalid;
   ResClass = DXIL::ResourceClass::Invalid;
-  if (PHINode *Phi = dyn_cast<PHINode>(handle)) {
+  if (!isa<CallInst>(handle)) {
     DenseSet<Value *> Checked;
-    handle = GetHandleFromPhi(Phi, Checked);
-    if (!handle) {
+    if (CallInst *CI = GetHandleFromValue(handle, Checked)) {
+      handle = CI;
+    } else {
       ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
       return DXIL::ResourceKind::Invalid;
     }
   }
   // TODO: validate ROV is used only in PS.
-  if (!isa<CallInst>(handle)) {
-    ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
-    return DXIL::ResourceKind::Invalid;
-  }
 
   DxilInst_CreateHandle createHandle(cast<CallInst>(handle));
   if (!createHandle) {
@@ -1218,20 +1230,16 @@ DxilResourceBase *ValidationContext::GetResourceFromVal(Value *resVal) {
 }
 
 static DxilResource *GetResource(Value *handle, ValidationContext &ValCtx) {
-  if (PHINode *Phi = dyn_cast<PHINode>(handle)) {
+  if (!isa<CallInst>(handle)) {
     DenseSet<Value *> Checked;
-    handle = GetHandleFromPhi(Phi, Checked);
-    if (!handle) {
+    if (CallInst *CI = GetHandleFromValue(handle, Checked)) {
+      handle = CI;
+    } else {
       ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
       return nullptr;
     }
   }
 
-  if (!isa<CallInst>(handle)) {
-    ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
-    return nullptr;
-  }
-
   DxilInst_CreateHandle createHandle(cast<CallInst>(handle));
   if (!createHandle) {
     auto EmitError = [&]() -> DXIL::ResourceKind {
@@ -1551,12 +1559,12 @@ static unsigned StoreValueToMask(ArrayRef<Value *> vals) {
 }
 
 static int GetCBufSize(Value *cbHandle, ValidationContext &ValCtx) {
-  if (PHINode *Phi = dyn_cast<PHINode>(cbHandle)) {
+  if (!isa<CallInst>(cbHandle)) {
     DenseSet<Value *> Checked;
-    cbHandle = GetHandleFromPhi(Phi, Checked);
-    if (!cbHandle) {
-      ValCtx.EmitInstrError(Phi,
-                            ValidationRule::InstrHandleNotFromCreateHandle);
+    if (CallInst *CI = GetHandleFromValue(cbHandle, Checked)) {
+      cbHandle = CI;
+    } else {
+      ValCtx.EmitError(ValidationRule::InstrHandleNotFromCreateHandle);
       return -1;
     }
   }

tools/clang/test/CodeGenHLSL/quick-test/res_select2.hlsl

@@ -1,7 +1,14 @@
 // RUN: %dxc -T lib_6_3 -auto-binding-space 11 %s | FileCheck %s
 
-// Make sure phi of resource for lib.
-// CHECK: phi %"class.RWBuffer
+// Make sure phi/select of handle in lib.
+// CHECK-DAG: phi %dx.types.Handle
+// CHECK: phi %dx.types.Handle
+// CHECK: select i1 %{{[^,]+}}, %dx.types.Handle
+// CHECK-DAG: phi %dx.types.Handle
+// CHECK: phi %dx.types.Handle
+// CHECK: select i1 %{{[^,]+}}, %dx.types.Handle
+// CHECK-DAG: phi %dx.types.Handle
+// CHECK: phi %dx.types.Handle
 
 RWBuffer<float4> a;
 RWBuffer<float4> b;

tools/clang/test/CodeGenHLSL/quick-test/res_select3.hlsl

@@ -1,7 +1,14 @@
 // RUN: %dxc -T lib_6_3 -auto-binding-space 11 %s | FileCheck %s
 
-// Make sure phi of resource in lib.
-// CHECK: phi %"class.RWStructuredBuffer
+// Make sure phi/select of handle in lib.
+// CHECK-DAG: phi %dx.types.Handle
+// CHECK: phi %dx.types.Handle
+// CHECK: select i1 %{{[^,]+}}, %dx.types.Handle
+// CHECK-DAG: phi %dx.types.Handle
+// CHECK: phi %dx.types.Handle
+// CHECK: select i1 %{{[^,]+}}, %dx.types.Handle
+// CHECK-DAG: phi %dx.types.Handle
+// CHECK: phi %dx.types.Handle
 
 // Make sure get dimensions returns 24
 // CHECK: ret i32 24