From 1585d6d950be643e5e3ba9741b984a2eeb2b33d2 Mon Sep 17 00:00:00 2001
From: Joshua Batista <jbatista@microsoft.com>
Date: Tue, 19 Dec 2023 11:47:36 -0800
Subject: [PATCH] address more feedback

---
 lib/HLSL/DxilValidation.cpp | 80 +++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 44 deletions(-)

diff --git a/lib/HLSL/DxilValidation.cpp b/lib/HLSL/DxilValidation.cpp
index 39c77c9d9c..b12561b724 100644
--- a/lib/HLSL/DxilValidation.cpp
+++ b/lib/HLSL/DxilValidation.cpp
@@ -2136,19 +2136,17 @@ static void ValidateBarrierFlagArg(ValidationContext &ValCtx, CallInst *CI,
 
 std::string GetLaunchTypeStr(DXIL::NodeLaunchType LT) {
   switch (LT) {
-	case DXIL::NodeLaunchType::Broadcasting:
-		return "Broadcasting";
+  case DXIL::NodeLaunchType::Broadcasting:
+    return "Broadcasting";
   case DXIL::NodeLaunchType::Coalescing:
-		return "Coalescing";
+    return "Coalescing";
   case DXIL::NodeLaunchType::Thread:
     return "Thread";
   case DXIL::NodeLaunchType::Invalid:
   case DXIL::NodeLaunchType::LastEntry:
     return "Invalid";
-
-  default:
-    return "Invalid";
   }
+  llvm_unreachable("invalid launch type");
 }
 
 static void ValidateDxilOperationCallInProfile(CallInst *CI,
@@ -2161,8 +2159,7 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI,
   DXIL::NodeLaunchType nodeLaunchType = DXIL::NodeLaunchType::Invalid;
   if (DXIL::ShaderKind::Library == shaderKind) {
     if (ValCtx.DxilMod.HasDxilFunctionProps(F)) {
-      DxilModule &DM = ValCtx.DxilMod;
-      DxilEntryProps &entryProps = DM.GetDxilEntryProps(F);
+      DxilEntryProps &entryProps = ValCtx.DxilMod.GetDxilEntryProps(F);
       nodeLaunchType = entryProps.props.Node.LaunchType;
       shaderKind = ValCtx.DxilMod.GetDxilFunctionProps(F).shaderKind;
     } else if (ValCtx.DxilMod.IsPatchConstantShader(F))
@@ -2177,8 +2174,6 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI,
   // Is called from a library function
   bool isLibFunc = shaderKind == DXIL::ShaderKind::Library;
 
-  // get the node launch type
-
   ValidateHandleArgs(CI, opcode, ValCtx);
 
   switch (opcode) {
@@ -2375,60 +2370,57 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI,
       break;
     }
 
-    if (nodeLaunchType == DXIL::NodeLaunchType::Broadcasting) {
-      // good
-    } else {
-      ValCtx.EmitInstrFormatError(CI,
-                                  ValidationRule::InstrSVConflictingLaunchMode,
-                                  {"ThreadId", "SV_DispatchThreadID",
-                                   GetLaunchTypeStr(nodeLaunchType)});
-    }
+    if (nodeLaunchType == DXIL::NodeLaunchType::Broadcasting)
+      break;
+
+    ValCtx.EmitInstrFormatError(
+        CI, ValidationRule::InstrSVConflictingLaunchMode,
+        {"ThreadId", "SV_DispatchThreadID", GetLaunchTypeStr(nodeLaunchType)});
   } break;
-  
+
   case DXIL::OpCode::GroupId: { // SV_GroupId
     if (shaderKind != DXIL::ShaderKind::Node) {
       break;
     }
 
-    if (nodeLaunchType == DXIL::NodeLaunchType::Broadcasting) {
-      // good
-    } else {
-      ValCtx.EmitInstrFormatError(
-          CI, ValidationRule::InstrSVConflictingLaunchMode,
-          {"GroupId", "SV_GroupId", GetLaunchTypeStr(nodeLaunchType)});
-    }
+    if (nodeLaunchType == DXIL::NodeLaunchType::Broadcasting)
+      break;
+
+    ValCtx.EmitInstrFormatError(
+        CI, ValidationRule::InstrSVConflictingLaunchMode,
+        {"GroupId", "SV_GroupId", GetLaunchTypeStr(nodeLaunchType)});
   } break;
-  
+
   case DXIL::OpCode::ThreadIdInGroup: { // SV_GroupThreadID
     if (shaderKind != DXIL::ShaderKind::Node) {
       break;
     }
 
     if (nodeLaunchType == DXIL::NodeLaunchType::Broadcasting ||
-        nodeLaunchType == DXIL::NodeLaunchType::Coalescing) {
-      // good
-    } else {
-      ValCtx.EmitInstrFormatError(CI,
-                                  ValidationRule::InstrSVConflictingLaunchMode,
-                                  {"ThreadIdInGroup", "SV_GroupThreadID",
-                                   GetLaunchTypeStr(nodeLaunchType)});
-    }
+        nodeLaunchType == DXIL::NodeLaunchType::Coalescing)
+      break;
+
+    ValCtx.EmitInstrFormatError(CI,
+                                ValidationRule::InstrSVConflictingLaunchMode,
+                                {"ThreadIdInGroup", "SV_GroupThreadID",
+                                 GetLaunchTypeStr(nodeLaunchType)});
+
   } break;
-  
+
   case DXIL::OpCode::FlattenedThreadIdInGroup: { // SV_GroupIndex
     if (shaderKind != DXIL::ShaderKind::Node) {
       break;
     }
 
     if (nodeLaunchType == DXIL::NodeLaunchType::Broadcasting ||
-        nodeLaunchType == DXIL::NodeLaunchType::Coalescing) {
-      // good
-    } else {
-      ValCtx.EmitInstrFormatError(CI,
-                                  ValidationRule::InstrSVConflictingLaunchMode,
-                                  {"FlattenedThreadIdInGroup", "SV_GroupIndex",
-                                   GetLaunchTypeStr(nodeLaunchType)});
-    }
+        nodeLaunchType == DXIL::NodeLaunchType::Coalescing)
+      break;
+
+    ValCtx.EmitInstrFormatError(CI,
+                                ValidationRule::InstrSVConflictingLaunchMode,
+                                {"FlattenedThreadIdInGroup", "SV_GroupIndex",
+                                 GetLaunchTypeStr(nodeLaunchType)});
+
   } break;
   default:
     // TODO: make sure every opcode is checked.