From 7b97fadd2e3f2c9c1348aaa86e101d4e4c777b7b Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Thu, 14 Dec 2023 16:36:28 -0800 Subject: [PATCH] address feedback --- docs/DXIL.rst | 2 +- lib/HLSL/DxilValidation.cpp | 243 +++++++++--------- .../dxil/intrinsics/launch_types_broadcast.ll | 2 - .../intrinsics/launch_types_coalescing.ll | 2 - .../dxil/intrinsics/launch_types_thread.ll | 2 - utils/hct/hctdb.py | 6 +- 6 files changed, 128 insertions(+), 129 deletions(-) diff --git a/docs/DXIL.rst b/docs/DXIL.rst index 31070b7e1d..c31c150027 100644 --- a/docs/DXIL.rst +++ b/docs/DXIL.rst @@ -3071,7 +3071,6 @@ INSTR.FAILTORESLOVETGSMPOINTER TGSM pointers must originate from an u INSTR.HANDLENOTFROMCREATEHANDLE Resource handle should returned by createHandle. INSTR.IMMBIASFORSAMPLEB bias amount for sample_b must be in the range [%0,%1], but %2 was specified as an immediate. INSTR.INBOUNDSACCESS Access to out-of-bounds memory is disallowed. -INSTR.INVALIDSVINFUNCTION An SV value was used on a parameter to a node shader function that isn't compatible. INSTR.MINPRECISIONNOTPRECISE Instructions marked precise may not refer to minprecision values. INSTR.MINPRECISONBITCAST Bitcast on minprecison types is not allowed. INSTR.MIPLEVELFORGETDIMENSION Use mip level on buffer when GetDimensions. @@ -3125,6 +3124,7 @@ INSTR.SAMPLERMODEFORSAMPLEC sample_c_*/gather_c instructions requi INSTR.SIGNATUREOPERATIONNOTINENTRY Dxil operation for input output signature must be in entryPoints. INSTR.STATUS Resource status should only be used by CheckAccessFullyMapped. INSTR.STRUCTBITCAST Bitcast on struct types is not allowed. +INSTR.SVCONFLICTINGLAUNCHMODE Input system values are compatible with node shader launch mode. INSTR.TEXTUREOFFSET offset texture instructions must take offset which can resolve to integer literal in the range -8 to 7. INSTR.TGSMRACECOND Race condition writing to shared memory detected, consider making this write conditional. INSTR.UNDEFINEDVALUEFORUAVSTORE Assignment of undefined values to UAV. diff --git a/lib/HLSL/DxilValidation.cpp b/lib/HLSL/DxilValidation.cpp index 9765a484b1..ea85a478d0 100644 --- a/lib/HLSL/DxilValidation.cpp +++ b/lib/HLSL/DxilValidation.cpp @@ -2134,6 +2134,24 @@ static void ValidateBarrierFlagArg(ValidationContext &ValCtx, CallInst *CI, } } +std::string GetLaunchTypeStr(DXIL::NodeLaunchType LT) { + switch (LT) { + case DXIL::NodeLaunchType::Broadcasting: + return "Broadcasting"; + case DXIL::NodeLaunchType::Coalescing: + return "Coalescing"; + case DXIL::NodeLaunchType::Thread: + return "Thread"; + case DXIL::NodeLaunchType::Invalid: + return "Invalid"; + case DXIL::NodeLaunchType::LastEntry: + return "LastEntry"; + + default: + return "Unknown"; + } +} + static void ValidateDxilOperationCallInProfile(CallInst *CI, DXIL::OpCode opcode, const ShaderModel *pSM, @@ -2156,6 +2174,15 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI, // Is called from a library function bool isLibFunc = shaderKind == DXIL::ShaderKind::Library; + // get the node launch type + DxilModule &DM = ValCtx.DxilMod; + DXIL::NodeLaunchType NLT = DXIL::NodeLaunchType::Invalid; + + if (DM.HasDxilEntryProps(F)) { + DxilEntryProps &entryProps = DM.GetDxilEntryProps(F); + NLT = entryProps.props.Node.LaunchType; + } + ValidateHandleArgs(CI, opcode, ValCtx); switch (opcode) { @@ -2346,80 +2373,62 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI, {"CreateHandle", "Shader model 6.5 and below"}); } } break; - - // this intrinsic is generated when the SV_DispatchThreadID system value is used - case DXIL::OpCode::ThreadId: { - // get the node launch type - DxilModule &DM = ValCtx.DxilMod; - if (!DM.HasDxilEntryProps(F)) { + + case DXIL::OpCode::ThreadId: { // SV_DispatchThreadID + if (shaderKind != DXIL::ShaderKind::Node) { break; } - DxilEntryProps &entryProps = DM.GetDxilEntryProps(F); - DXIL::NodeLaunchType NLT = entryProps.props.Node.LaunchType; - if (NLT == DXIL::NodeLaunchType::Thread || NLT == DXIL::NodeLaunchType::Coalescing) { - ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrInvalidSVInFunction, - {"ThreadId", NLT == - DXIL::NodeLaunchType::Thread - ? "Thread" - : "Coalescing"}); - } - } break; - // this intrinsic is generated when the SV_GroupId - // system value is used + if (NLT == DXIL::NodeLaunchType::Broadcasting) { + // good + } else { + ValCtx.EmitInstrFormatError( + CI, ValidationRule::InstrSVConflictingLaunchMode, + {"ThreadId", "SV_DispatchThreadID", GetLaunchTypeStr(NLT)}); + } + } break; - case DXIL::OpCode::GroupId: { - // get the node launch type - DxilModule &DM = ValCtx.DxilMod; - if (!DM.HasDxilEntryProps(F)) { + case DXIL::OpCode::GroupId: { // SV_GroupId + if (shaderKind != DXIL::ShaderKind::Node) { break; } - DxilEntryProps &entryProps = DM.GetDxilEntryProps(F); - DXIL::NodeLaunchType NLT = entryProps.props.Node.LaunchType; - if (NLT == DXIL::NodeLaunchType::Thread || NLT == DXIL::NodeLaunchType::Coalescing) { - ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrInvalidSVInFunction, - {"GroupId", NLT == - DXIL::NodeLaunchType::Thread - ? "Thread" - : "Coalescing"}); - } - } - break; - - // this intrinsic is generated when the SV_GroupThreadID - // system value is used - case DXIL::OpCode::ThreadIdInGroup: { - // get the node launch type - DxilModule &DM = ValCtx.DxilMod; - if (!DM.HasDxilEntryProps(F)) { - break; + if (NLT == DXIL::NodeLaunchType::Broadcasting) { + // good + } else { + ValCtx.EmitInstrFormatError( + CI, ValidationRule::InstrSVConflictingLaunchMode, + {"GroupId", "SV_GroupId", GetLaunchTypeStr(NLT)}); + } + } break; + + case DXIL::OpCode::ThreadIdInGroup: { // SV_GroupThreadID + if (shaderKind != DXIL::ShaderKind::Node) { + break; } - DxilEntryProps &entryProps = DM.GetDxilEntryProps(F); - DXIL::NodeLaunchType NLT = entryProps.props.Node.LaunchType; - if (NLT == DXIL::NodeLaunchType::Thread) { - ValCtx.EmitInstrFormatError( - CI, ValidationRule::InstrInvalidSVInFunction, - {"ThreadIdInGroup", "Thread"}); + if (NLT == DXIL::NodeLaunchType::Broadcasting || + NLT == DXIL::NodeLaunchType::Coalescing) { + // good + } else { + ValCtx.EmitInstrFormatError( + CI, ValidationRule::InstrSVConflictingLaunchMode, + {"ThreadIdInGroup", "SV_GroupThreadID", GetLaunchTypeStr(NLT)}); } } break; - -// this intrinsic is generated when the SV_GroupIndex -// system value is used - case DXIL::OpCode::FlattenedThreadIdInGroup: { - // get the node launch type - DxilModule &DM = ValCtx.DxilMod; - if (!DM.HasDxilEntryProps(F)) { - break; + + case DXIL::OpCode::FlattenedThreadIdInGroup: { // SV_GroupIndex + if (shaderKind != DXIL::ShaderKind::Node) { + break; } - DxilEntryProps &entryProps = DM.GetDxilEntryProps(F); - DXIL::NodeLaunchType NLT = entryProps.props.Node.LaunchType; - if (NLT == DXIL::NodeLaunchType::Thread) { - ValCtx.EmitInstrFormatError( - CI, ValidationRule::InstrInvalidSVInFunction, - {"FlattenedThreadIdInGroup", "Thread"}); + if (NLT == DXIL::NodeLaunchType::Broadcasting || + NLT == DXIL::NodeLaunchType::Coalescing) { + // good + } else { + ValCtx.EmitInstrFormatError( + CI, ValidationRule::InstrSVConflictingLaunchMode, + {"FlattenedThreadIdInGroup", "SV_GroupIndex", GetLaunchTypeStr(NLT)}); } } break; default: @@ -3539,62 +3548,6 @@ static void ValidateNodeInputRecord(Function *F, ValidationContext &ValCtx) { } } -static void ValidateFunctionArgs(Function& F, unsigned numUDTShaderArgs, DXIL::ShaderKind shaderKind, ValidationContext& ValCtx) { - - auto ArgFormatError = [&](Function &F, Argument &arg, ValidationRule rule) { - if (arg.hasName()) - ValCtx.EmitFnFormatError(&F, rule, {arg.getName().str(), F.getName()}); - else - ValCtx.EmitFnFormatError(&F, rule, - {std::to_string(arg.getArgNo()), F.getName()}); - }; - - unsigned numArgs = 0; - for (auto &arg : F.args()) { - Type *argTy = arg.getType(); - if (argTy->isPointerTy()) - argTy = argTy->getPointerElementType(); - - numArgs++; - if (numUDTShaderArgs) { - if (arg.getArgNo() >= numUDTShaderArgs) { - ArgFormatError(F, arg, ValidationRule::DeclExtraArgs); - } else if (!argTy->isStructTy()) { - switch (shaderKind) { - case DXIL::ShaderKind::Callable: - ArgFormatError(F, arg, ValidationRule::DeclParamStruct); - break; - default: - ArgFormatError(F, arg, - arg.getArgNo() == 0 ? ValidationRule::DeclPayloadStruct - : ValidationRule::DeclAttrStruct); - } - } - continue; - } - - while (argTy->isArrayTy()) { - argTy = argTy->getArrayElementType(); - } - - if (argTy->isStructTy() && !ValCtx.isLibProfile) { - ArgFormatError(F, arg, ValidationRule::DeclFnFlattenParam); - break; - } - } - - if (numArgs < numUDTShaderArgs && shaderKind != DXIL::ShaderKind::Node) { - StringRef argType[2] = { - shaderKind == DXIL::ShaderKind::Callable ? "params" : "payload", - "attributes"}; - for (unsigned i = numArgs; i < numUDTShaderArgs; i++) { - ValCtx.EmitFnFormatError( - &F, ValidationRule::DeclShaderMissingArg, - {ShaderModel::GetKindName(shaderKind), F.getName(), argType[i]}); - } - } -} - static void ValidateFunction(Function &F, ValidationContext &ValCtx) { if (F.isDeclaration()) { ValidateExternalFunction(&F, ValCtx); @@ -3626,7 +3579,7 @@ static void ValidateFunction(Function &F, ValidationContext &ValCtx) { } } break; - } + } default: break; } @@ -3645,7 +3598,59 @@ static void ValidateFunction(Function &F, ValidationContext &ValCtx) { {F.getName()}); // Validate parameter type. - ValidateFunctionArgs(F, numUDTShaderArgs, shaderKind, ValCtx); + auto ArgFormatError = [&](Function &F, Argument &arg, ValidationRule rule) { + if (arg.hasName()) + ValCtx.EmitFnFormatError(&F, rule, {arg.getName().str(), F.getName()}); + else + ValCtx.EmitFnFormatError(&F, rule, + {std::to_string(arg.getArgNo()), F.getName()}); + }; + + unsigned numArgs = 0; + for (auto &arg : F.args()) { + Type *argTy = arg.getType(); + if (argTy->isPointerTy()) + argTy = argTy->getPointerElementType(); + + numArgs++; + if (numUDTShaderArgs) { + if (arg.getArgNo() >= numUDTShaderArgs) { + ArgFormatError(F, arg, ValidationRule::DeclExtraArgs); + } else if (!argTy->isStructTy()) { + switch (shaderKind) { + case DXIL::ShaderKind::Callable: + ArgFormatError(F, arg, ValidationRule::DeclParamStruct); + break; + default: + ArgFormatError(F, arg, + arg.getArgNo() == 0 + ? ValidationRule::DeclPayloadStruct + : ValidationRule::DeclAttrStruct); + } + } + continue; + } + + while (argTy->isArrayTy()) { + argTy = argTy->getArrayElementType(); + } + + if (argTy->isStructTy() && !ValCtx.isLibProfile) { + ArgFormatError(F, arg, ValidationRule::DeclFnFlattenParam); + break; + } + } + + if (numArgs < numUDTShaderArgs && shaderKind != DXIL::ShaderKind::Node) { + StringRef argType[2] = { + shaderKind == DXIL::ShaderKind::Callable ? "params" : "payload", + "attributes"}; + for (unsigned i = numArgs; i < numUDTShaderArgs; i++) { + ValCtx.EmitFnFormatError( + &F, ValidationRule::DeclShaderMissingArg, + {ShaderModel::GetKindName(shaderKind), F.getName(), argType[i]}); + } + } if (ValCtx.DxilMod.HasDxilFunctionProps(&F) && ValCtx.DxilMod.GetDxilFunctionProps(&F).IsNode()) { diff --git a/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_broadcast.ll b/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_broadcast.ll index 5e6b596aa9..974b337f00 100644 --- a/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_broadcast.ll +++ b/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_broadcast.ll @@ -46,5 +46,3 @@ declare i32 @dx.op.threadIdInGroup.i32(i32, i32) #0 ; Function Attrs: nounwind readnone declare i32 @dx.op.flattenedThreadIdInGroup.i32(i32) #0 - -; this file comes from compiling launch_types.hlsl with: diff --git a/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_coalescing.ll b/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_coalescing.ll index 36dd4ad467..6e1b969aeb 100644 --- a/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_coalescing.ll +++ b/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_coalescing.ll @@ -49,5 +49,3 @@ declare i32 @dx.op.threadIdInGroup.i32(i32, i32) #0 ; Function Attrs: nounwind readnone declare i32 @dx.op.flattenedThreadIdInGroup.i32(i32) #0 - -; this file comes from compiling launch_types.hlsl with: diff --git a/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_thread.ll b/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_thread.ll index fafdf89a3a..96d3b71ce8 100644 --- a/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_thread.ll +++ b/tools/clang/test/HLSLFileCheck/dxil/intrinsics/launch_types_thread.ll @@ -53,5 +53,3 @@ declare i32 @dx.op.threadIdInGroup.i32(i32, i32) #0 ; Function Attrs: nounwind readnone declare i32 @dx.op.flattenedThreadIdInGroup.i32(i32) #0 - -; this file comes from compiling launch_types.hlsl with: diff --git a/utils/hct/hctdb.py b/utils/hct/hctdb.py index a4cd957ba5..012f23c747 100644 --- a/utils/hct/hctdb.py +++ b/utils/hct/hctdb.py @@ -7553,9 +7553,9 @@ def build_valrules(self): "Instr.AtomicIntrinNonUAV", "Non-UAV destination to atomic intrinsic." ) self.add_valrule_msg( - "Instr.InvalidSVInFunction", - "An SV value was used on a parameter to a node shader function that isn't compatible.", - "Call to DXIL intrinsic %0 is not allowed in node shader launch type %1" + "Instr.SVConflictingLaunchMode", + "Input system values are compatible with node shader launch mode.", + "Call to DXIL intrinsic %0 (%1) is not allowed in node shader launch type %2" ) self.add_valrule("Instr.AtomicConst", "Constant destination to atomic.")