Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spirv-val: Add missing NonSemantic.Shader.DebugInfo.100 #5846

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

spirv-val: Add missing NonSemantic.Shader.DebugInfo.100 #5846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
085c93b
spirv-val: Cleanup DebugExtension tests
spencer-lunarg Oct 8, 2024
46d7384
spirv-val: Cleanup DebugExtension tests extensions
spencer-lunarg Oct 8, 2024
b7573f3
spirv-val: Add missing NonSemantic.Shader.DebugInfo.100
spencer-lunarg Oct 9, 2024
66815b5
spirv-opt: Fix broken DebugLine in tests
spencer-lunarg Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/val/function.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ class Function {
Construct& FindConstructForEntryBlock(const BasicBlock* entry_block,
ConstructType t);

/// The result id of the OpLabel that defined this block
/// The result id of OpFunction
uint32_t id_;

/// The type of the function
Expand Down
123 changes: 96 additions & 27 deletions source/val/validate_extensions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3090,7 +3090,6 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
// validation.
case NonSemanticShaderDebugInfo100DebugInfoNone:
case NonSemanticShaderDebugInfo100DebugCompilationUnit:
case NonSemanticShaderDebugInfo100DebugTypeBasic:
case NonSemanticShaderDebugInfo100DebugTypePointer:
case NonSemanticShaderDebugInfo100DebugTypeQualifier:
case NonSemanticShaderDebugInfo100DebugTypeArray:
Expand All @@ -3116,7 +3115,6 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
case NonSemanticShaderDebugInfo100DebugInlinedAt:
case NonSemanticShaderDebugInfo100DebugLocalVariable:
case NonSemanticShaderDebugInfo100DebugInlinedVariable:
case NonSemanticShaderDebugInfo100DebugDeclare:
case NonSemanticShaderDebugInfo100DebugValue:
case NonSemanticShaderDebugInfo100DebugOperation:
case NonSemanticShaderDebugInfo100DebugExpression:
Expand All @@ -3125,6 +3123,24 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
case NonSemanticShaderDebugInfo100DebugImportedEntity:
case NonSemanticShaderDebugInfo100DebugSource:
break;

// These checks are for operands that are differnet in
// ShaderDebugInfo100
case NonSemanticShaderDebugInfo100DebugTypeBasic: {
CHECK_CONST_UINT_OPERAND("Flags", 8);
break;
}
case NonSemanticShaderDebugInfo100DebugDeclare: {
for (uint32_t word_index = 8; word_index < num_words; ++word_index) {
auto index_inst = _.FindDef(inst->word(word_index));
auto type_id = index_inst != nullptr ? index_inst->type_id() : 0;
if (type_id == 0 || !IsIntScalar(_, type_id, false, false))
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< ext_inst_name() << ": "
<< "expected index must be scalar integer";
}
break;
}
case NonSemanticShaderDebugInfo100DebugTypeMatrix: {
CHECK_DEBUG_OPERAND("Vector Type", CommonDebugInfoDebugTypeVector, 5);

Expand All @@ -3146,14 +3162,83 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
}
break;
}
// TODO: Add validation rules for remaining cases as well.
case NonSemanticShaderDebugInfo100DebugFunctionDefinition:
case NonSemanticShaderDebugInfo100DebugSourceContinued:
case NonSemanticShaderDebugInfo100DebugLine:
case NonSemanticShaderDebugInfo100DebugFunctionDefinition: {
CHECK_DEBUG_OPERAND("Function", CommonDebugInfoDebugFunction, 5);
CHECK_OPERAND("Definition", spv::Op::OpFunction, 6);
const auto* current_function = inst->function();
if (current_function->first_block()->id() != inst->block()->id()) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< ext_inst_name()
<< ": must be in the entry basic block of the function";
}

const uint32_t definition_id = inst->word(6);
if (definition_id != current_function->id()) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< ext_inst_name()
<< ": operand Definition must point to the OpFunction it is "
"inside";
}
break;
}
case NonSemanticShaderDebugInfo100DebugLine: {
CHECK_DEBUG_OPERAND("Source", CommonDebugInfoDebugSource, 5);
CHECK_CONST_UINT_OPERAND("Line Start", 6);
CHECK_CONST_UINT_OPERAND("Line End", 7);
CHECK_CONST_UINT_OPERAND("Column Start", 8);
CHECK_CONST_UINT_OPERAND("Column End", 9);

// above already validates if 32-bit and non-spec constant
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tests with these as int64 and SpecConstant and confirmed this is true

// but want to use EvalInt32IfConst to be consistent with other Eval
// locations
bool is_int32 = false, is_const_int32 = false;
uint32_t line_start = 0;
uint32_t line_end = 0;
uint32_t column_start = 0;
uint32_t column_end = 0;
std::tie(is_int32, is_const_int32, line_start) =
_.EvalInt32IfConst(inst->word(6));
std::tie(is_int32, is_const_int32, line_end) =
_.EvalInt32IfConst(inst->word(7));
std::tie(is_int32, is_const_int32, column_start) =
_.EvalInt32IfConst(inst->word(8));
std::tie(is_int32, is_const_int32, column_end) =
_.EvalInt32IfConst(inst->word(9));
if (line_end < line_start) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< ext_inst_name() << ": operand Line End (" << line_end
<< ") is less than Line Start (" << line_start << ")";
} else if (column_end < column_start) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< ext_inst_name() << ": operand Column End (" << column_end
<< ") is less than Column Start (" << column_start << ")";
}
break;
}
case NonSemanticShaderDebugInfo100DebugSourceContinued: {
CHECK_OPERAND("Text", spv::Op::OpString, 5);
break;
}
case NonSemanticShaderDebugInfo100DebugBuildIdentifier: {
CHECK_OPERAND("Identifier", spv::Op::OpString, 5);
CHECK_CONST_UINT_OPERAND("Flags", 6);
break;
}
case NonSemanticShaderDebugInfo100DebugStoragePath: {
CHECK_OPERAND("Path", spv::Op::OpString, 5);
break;
}
case NonSemanticShaderDebugInfo100DebugEntryPoint: {
CHECK_DEBUG_OPERAND("Entry Point", CommonDebugInfoDebugFunction, 5);
CHECK_DEBUG_OPERAND("Compilation Unit",
CommonDebugInfoDebugCompilationUnit, 6);
CHECK_OPERAND("Compiler Signature", spv::Op::OpString, 7);
CHECK_OPERAND("Command-line Arguments", spv::Op::OpString, 8);
break;
}

// Has no additional checks
case NonSemanticShaderDebugInfo100DebugNoLine:
case NonSemanticShaderDebugInfo100DebugBuildIdentifier:
case NonSemanticShaderDebugInfo100DebugStoragePath:
case NonSemanticShaderDebugInfo100DebugEntryPoint:
break;
case NonSemanticShaderDebugInfo100InstructionsMax:
assert(0);
Expand Down Expand Up @@ -3455,9 +3540,7 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
}
case CommonDebugInfoDebugFunction: {
CHECK_OPERAND("Name", spv::Op::OpString, 5);
auto validate_type = ValidateOperandDebugType(_, "Type", inst, 6,
ext_inst_name, false);
if (validate_type != SPV_SUCCESS) return validate_type;
CHECK_DEBUG_OPERAND("Type", CommonDebugInfoDebugTypeFunction, 6);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just looks like a copy-and-paste error, the other locations need to check the type of the instruction, but these are specifically just DebugTypeFunction

CHECK_DEBUG_OPERAND("Source", CommonDebugInfoDebugSource, 7);
CHECK_CONST_UINT_OPERAND("Line", 8);
CHECK_CONST_UINT_OPERAND("Column", 9);
Expand Down Expand Up @@ -3492,9 +3575,7 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
}
case CommonDebugInfoDebugFunctionDeclaration: {
CHECK_OPERAND("Name", spv::Op::OpString, 5);
auto validate_type = ValidateOperandDebugType(_, "Type", inst, 6,
ext_inst_name, false);
if (validate_type != SPV_SUCCESS) return validate_type;
CHECK_DEBUG_OPERAND("Type", CommonDebugInfoDebugTypeFunction, 6);
CHECK_DEBUG_OPERAND("Source", CommonDebugInfoDebugSource, 7);
CHECK_CONST_UINT_OPERAND("Line", 8);
CHECK_CONST_UINT_OPERAND("Column", 9);
Expand Down Expand Up @@ -3556,18 +3637,6 @@ spv_result_t ValidateExtInst(ValidationState_t& _, const Instruction* inst) {
}

CHECK_DEBUG_OPERAND("Expression", CommonDebugInfoDebugExpression, 7);

if (vulkanDebugInfo) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved up above with the rest of the ShaderDebug code

for (uint32_t word_index = 8; word_index < num_words;
++word_index) {
auto index_inst = _.FindDef(inst->word(word_index));
auto type_id = index_inst != nullptr ? index_inst->type_id() : 0;
if (type_id == 0 || !IsIntScalar(_, type_id, false, false))
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< ext_inst_name() << ": "
<< "expected index must be scalar integer";
}
}
break;
}
case CommonDebugInfoDebugExpression: {
Expand Down
4 changes: 2 additions & 2 deletions source/val/validate_layout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ spv_result_t ModuleScopedInstructions(ValidationState_t& _,

if (local_debug_info) {
if (_.in_function_body() == false) {
// DebugScope, DebugNoScope, DebugDeclare, DebugValue must
// appear in a function body.
// TODO - Print the actual name of the instruction as this list is
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will make a new PR here, likely will require cleaning up some tests error strings so didn't want to do it here

// not complete (see ext_inst_name in ValidateExtInst() for example)
return _.diag(SPV_ERROR_INVALID_LAYOUT, inst)
<< "DebugScope, DebugNoScope, DebugDeclare, DebugValue "
<< "of debug info extension must appear in a function "
Expand Down
6 changes: 3 additions & 3 deletions test/opt/inline_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3749,13 +3749,13 @@ float4 main(float4 color : COLOR) : SV_TARGET {
%color = OpFunctionParameter %_ptr_Function_v4float
%bb_entry = OpLabel
%140 = OpExtInst %void %1 DebugFunctionDefinition %22 %src_main
%141 = OpExtInst %void %1 DebugLine %5 %uint_1 %uint_1 %uint_1 %uint_1
%141 = OpExtInst %void %1 DebugLine %15 %uint_1 %uint_1 %uint_1 %uint_1
%34 = OpExtInst %void %1 DebugScope %22
%36 = OpExtInst %void %1 DebugDeclare %25 %color %13
%38 = OpExtInst %void %1 DebugScope %26
%142 = OpExtInst %void %1 DebugLine %5 %uint_2 %uint_2 %uint_10 %uint_10
%142 = OpExtInst %void %1 DebugLine %15 %uint_2 %uint_2 %uint_10 %uint_10
%39 = OpLoad %v4float %color
%143 = OpExtInst %void %1 DebugLine %5 %uint_2 %uint_2 %uint_3 %uint_3
%143 = OpExtInst %void %1 DebugLine %15 %uint_2 %uint_2 %uint_3 %uint_3
OpReturnValue %39
OpFunctionEnd
)";
Expand Down
18 changes: 9 additions & 9 deletions test/opt/loop_optimizations/unroll_simple.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,36 +510,36 @@ OpBranch %24
%24 = OpLabel
%35 = OpPhi %8 %10 %23 %34 %26
%s1 = OpExtInst %6 %ext DebugScope %dbg_main
%d10 = OpExtInst %6 %ext DebugLine %file_name %uint_1 %uint_1 %uint_0 %uint_0
%d10 = OpExtInst %6 %ext DebugLine %src %uint_1 %uint_1 %uint_0 %uint_0
%value0 = OpExtInst %6 %ext DebugValue %dbg_f %35 %null_expr
OpLoopMerge %25 %26 Unroll
OpBranch %27
%27 = OpLabel
%s2 = OpExtInst %6 %ext DebugScope %dbg_main
%d1 = OpExtInst %6 %ext DebugLine %file_name %uint_1 %uint_1 %uint_1 %uint_1
%d1 = OpExtInst %6 %ext DebugLine %src %uint_1 %uint_1 %uint_1 %uint_1
%29 = OpSLessThan %12 %35 %11
%d2 = OpExtInst %6 %ext DebugLine %file_name %uint_2 %uint_2 %uint_0 %uint_0
%d2 = OpExtInst %6 %ext DebugLine %src %uint_2 %uint_2 %uint_0 %uint_0
OpBranchConditional %29 %30 %25
%30 = OpLabel
%s3 = OpExtInst %6 %ext DebugScope %bb
%decl0 = OpExtInst %6 %ext DebugDeclare %dbg_f %5 %null_expr
%decl1 = OpExtInst %6 %ext DebugValue %dbg_i %5 %deref_expr
%d3 = OpExtInst %6 %ext DebugLine %file_name %uint_3 %uint_3 %uint_0 %uint_0
%d3 = OpExtInst %6 %ext DebugLine %src %uint_3 %uint_3 %uint_0 %uint_0
%32 = OpAccessChain %19 %5 %35
%d4 = OpExtInst %6 %ext DebugLine %file_name %uint_4 %uint_4 %uint_0 %uint_0
%d4 = OpExtInst %6 %ext DebugLine %src %uint_4 %uint_4 %uint_0 %uint_0
OpStore %32 %18
%d5 = OpExtInst %6 %ext DebugLine %file_name %uint_5 %uint_5 %uint_0 %uint_0
%d5 = OpExtInst %6 %ext DebugLine %src %uint_5 %uint_5 %uint_0 %uint_0
OpBranch %26
%26 = OpLabel
%s4 = OpExtInst %6 %ext DebugScope %dbg_main
%d6 = OpExtInst %6 %ext DebugLine %file_name %uint_6 %uint_6 %uint_0 %uint_0
%d6 = OpExtInst %6 %ext DebugLine %src %uint_6 %uint_6 %uint_0 %uint_0
%34 = OpIAdd %8 %35 %20
%value1 = OpExtInst %6 %ext DebugValue %dbg_f %34 %null_expr
%d7 = OpExtInst %6 %ext DebugLine %file_name %uint_7 %uint_7 %uint_0 %uint_0
%d7 = OpExtInst %6 %ext DebugLine %src %uint_7 %uint_7 %uint_0 %uint_0
OpBranch %24
%25 = OpLabel
%s5 = OpExtInst %6 %ext DebugScope %dbg_main
%d8 = OpExtInst %6 %ext DebugLine %file_name %uint_8 %uint_8 %uint_0 %uint_0
%d8 = OpExtInst %6 %ext DebugLine %src %uint_8 %uint_8 %uint_0 %uint_0
OpReturn
OpFunctionEnd)";

Expand Down
Loading
Loading