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.

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

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

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

Conversation

spencer-lunarg
Copy link
Contributor

Adds missing coverage for NonSemantic.Shader.DebugInfo.100 where it was different from OpenCL.DebugInfo.100

(Created 2 commits prior to reduce the copy-and-pasting in the tests)

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

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

@@ -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

@@ -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

}

// TODO - Need to track in function scope
TEST_F(ValidateVulkan100DebugInfo, DISABLED_DebugFunctionDefinitionDuplicate) {
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 (and one below) I added, but marked as DISABLED because it will require some tracking in the Function and wanted more time how to do that without having to add cost to everyone no using DebugInfo

@spencer-lunarg
Copy link
Contributor Author

@alan-baker I see it is failing CI in https://btx.cloud.google.com/invocations/36895f7b-5b6d-4dac-8efe-c9225e6f04d6/targets/graphics_shader_compiler%2Fspirv-tools%2Fshaderc-smoketest%2Fpresubmit/log but these errors make no sense, locally it runs fine and all the errors are around not being able to run spirv-as on thing, not sure if you have seen this before

@alan-baker
Copy link
Contributor

Can you rebase on main? SPIRV-Headers merged a PR to remove aliases as separate grammar entries. The tooling is up-to-date on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants