Skip to content

Commit

Permalink
layers: Add warning for using Pointer in shader interface
Browse files Browse the repository at this point in the history
  • Loading branch information
spencer-lunarg committed Jan 14, 2025
1 parent 336c974 commit cdbc347
Show file tree
Hide file tree
Showing 7 changed files with 257 additions and 73 deletions.
13 changes: 7 additions & 6 deletions layers/core_checks/cc_shader_interface.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* Copyright (c) 2015-2024 The Khronos Group Inc.
* Copyright (c) 2015-2024 Valve Corporation
* Copyright (c) 2015-2024 LunarG, Inc.
* Copyright (C) 2015-2024 Google Inc.
/* Copyright (c) 2015-2025 The Khronos Group Inc.
* Copyright (c) 2015-2025 Valve Corporation
* Copyright (c) 2015-2025 LunarG, Inc.
* Copyright (C) 2015-2025 Google Inc.
* Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -513,16 +513,17 @@ bool CoreChecks::ValidateInterfaceBetweenStages(const spirv::Module &producer, c

for (const auto &[interface_slot, stage_variable] : producer_entrypoint.output_interface_slots) {
auto &slot = slot_map[interface_slot.Location()][interface_slot.Component()];
if (stage_variable->nested_struct || stage_variable->physical_storage_buffer) {
if (stage_variable->nested_struct) {
return skip; // TODO workaround
}
slot.output = stage_variable;
slot.output_type = interface_slot.type;
slot.output_width = interface_slot.bit_width;
}

for (const auto &[interface_slot, stage_variable] : consumer_entrypoint.input_interface_slots) {
auto &slot = slot_map[interface_slot.Location()][interface_slot.Component()];
if (stage_variable->nested_struct || stage_variable->physical_storage_buffer) {
if (stage_variable->nested_struct) {
return skip; // TODO workaround
}
slot.input = stage_variable;
Expand Down
29 changes: 25 additions & 4 deletions layers/core_checks/cc_spirv.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* Copyright (c) 2015-2024 The Khronos Group Inc.
* Copyright (c) 2015-2024 Valve Corporation
* Copyright (c) 2015-2024 LunarG, Inc.
* Copyright (C) 2015-2024 Google Inc.
/* Copyright (c) 2015-2025 The Khronos Group Inc.
* Copyright (c) 2015-2025 Valve Corporation
* Copyright (c) 2015-2025 LunarG, Inc.
* Copyright (C) 2015-2025 Google Inc.
* Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -1386,6 +1386,26 @@ bool CoreChecks::ValidateShaderFloatControl(const spirv::Module &module_state, c
return skip;
}

bool CoreChecks::ValidatePhysicalStorageBuffers(const spirv::Module &module_state, const spirv::EntryPoint &entrypoint,
const Location &loc) const {
bool skip = false;

// https://gitlab.khronos.org/spirv/SPIR-V/-/issues/779
// This will likely not get resolved for awhile as it will be a LOT of testing and currently it "just works" for simple cases,
// but should warn users if trying to do this.
// We need to check at vkCreateShaderModule time as we have found some drivers will crash here (from our VVL tests).
if (entrypoint.has_physical_storage_buffer_interface) {
skip |= LogWarning(
"WARNING-PhysicalStorageBuffer-interface", module_state.handle(), loc,
"(SPIR-V Interface) Is trying to use PhysicalStorageBuffer as an Input/Output User-Defined Variable in (%s). This "
"has unresolved specification discussion and is undefined and caution should be taken. Advice is to use "
"int64 or uvec2 instead to pass the pointer betweeen stages.",
string_VkShaderStageFlagBits(entrypoint.stage));
}

return skip;
}

bool CoreChecks::ValidateExecutionModes(const spirv::Module &module_state, const spirv::EntryPoint &entrypoint,
const spirv::StatelessData &stateless_data, const Location &loc) const {
bool skip = false;
Expand Down Expand Up @@ -3135,6 +3155,7 @@ bool CoreChecks::ValidateSpirvStateless(const spirv::Module &module_state, const
skip |= ValidateShaderStageInputOutputLimits(module_state, *entry_point, stateless_data, loc);
skip |= ValidateShaderFloatControl(module_state, *entry_point, stateless_data, loc);
skip |= ValidateExecutionModes(module_state, *entry_point, stateless_data, loc);
skip |= ValidatePhysicalStorageBuffers(module_state, *entry_point, loc);
skip |= ValidateConservativeRasterization(module_state, *entry_point, stateless_data, loc);
if (enabled_features.transformFeedback) {
skip |= ValidateTransformFeedbackEmitStreams(module_state, *entry_point, stateless_data, loc);
Expand Down
2 changes: 2 additions & 0 deletions layers/core_checks/core_validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,8 @@ class CoreChecks : public ValidationStateTracker {
const spirv::StatelessData& stateless_data, const Location& loc) const;
bool ValidateExecutionModes(const spirv::Module& module_state, const spirv::EntryPoint& entrypoint,
const spirv::StatelessData& stateless_data, const Location& loc) const;
bool ValidatePhysicalStorageBuffers(const spirv::Module& module_state, const spirv::EntryPoint& entrypoint,
const Location& loc) const;
bool ValidateShaderExecutionModes(const spirv::Module& module_state, const spirv::EntryPoint& entrypoint,
VkShaderStageFlagBits stage, const vvl::Pipeline* pipeline, const Location& loc) const;
bool ValidateInterfaceVertexInput(const vvl::Pipeline& pipeline, const spirv::Module& module_state,
Expand Down
24 changes: 15 additions & 9 deletions layers/state_tracker/shader_module.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (c) 2021-2024 The Khronos Group Inc.
/* Copyright (c) 2021-2025 The Khronos Group Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -24,6 +24,7 @@
#include <spirv/1.2/GLSL.std.450.h>
#include <spirv/unified1/NonSemanticShaderDebugInfo100.h>
#include "error_message/spirv_logging.h"
#include "utils/vk_layer_utils.h"

namespace spirv {

Expand Down Expand Up @@ -821,9 +822,13 @@ EntryPoint::EntryPoint(const Module& module_state, const Instruction& entrypoint
} else {
user_defined_interface_variables.push_back(&variable);

if (variable.base_type.StorageClass() == spv::StorageClassPhysicalStorageBuffer) {
has_physical_storage_buffer_interface = true;
}

// After creating, make lookup table
if (variable.interface_slots.empty()) {
continue;
continue; // will skip for things like PhysicalStorageBuffer
}
for (const auto& slot : variable.interface_slots) {
if (variable.storage_class == spv::StorageClassInput) {
Expand Down Expand Up @@ -1810,6 +1815,11 @@ std::vector<InterfaceSlot> StageInterfaceVariable::GetInterfaceSlots(StageInterf
return slots;
}

if (variable.base_type.StorageClass() == spv::StorageClassPhysicalStorageBuffer) {
// PhysicalStorageBuffer interfaces not supported (https://gitlab.khronos.org/spirv/SPIR-V/-/issues/779)
return slots;
}

if (variable.type_struct_info) {
// Structs has two options being labeled
// 1. The block is given a Location, need to walk though and add up starting for that value
Expand All @@ -1824,11 +1834,8 @@ std::vector<InterfaceSlot> StageInterfaceVariable::GetInterfaceSlots(StageInterf

// Info needed to test type matching later
const Instruction* numerical_type = module_state.GetBaseTypeInstruction(member_id);
// TODO 5374 - Handle PhysicalStorageBuffer interfaces
if (!numerical_type) {
variable.physical_storage_buffer = true;
break;
}
ASSERT_AND_CONTINUE(numerical_type);

const uint32_t numerical_type_opcode = numerical_type->Opcode();
// TODO - Handle nested structs
if (numerical_type_opcode == spv::OpTypeStruct) {
Expand Down Expand Up @@ -1875,7 +1882,7 @@ std::vector<InterfaceSlot> StageInterfaceVariable::GetInterfaceSlots(StageInterf
} else {
uint32_t locations = 0;
// Will have array peeled off already
uint32_t type_id = variable.base_type.ResultId();
const uint32_t type_id = variable.base_type.ResultId();

locations = module_state.GetLocationsConsumedByType(type_id);
const uint32_t components = module_state.GetComponentsConsumedByType(type_id);
Expand Down Expand Up @@ -1941,7 +1948,6 @@ StageInterfaceVariable::StageInterfaceVariable(const Module& module_state, const
base_type(FindBaseType(*this, module_state)),
is_builtin(IsBuiltin(*this, module_state)),
nested_struct(false),
physical_storage_buffer(false),
interface_slots(GetInterfaceSlots(*this, module_state)),
builtin_block(GetBuiltinBlock(*this, module_state)),
total_builtin_components(GetBuiltinComponents(*this, module_state)) {}
Expand Down
5 changes: 3 additions & 2 deletions layers/state_tracker/shader_module.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (c) 2021-2024 The Khronos Group Inc.
/* Copyright (c) 2021-2025 The Khronos Group Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -359,7 +359,6 @@ struct StageInterfaceVariable : public VariableBase {
const Instruction &base_type;
const bool is_builtin;
bool nested_struct;
bool physical_storage_buffer;

const std::vector<InterfaceSlot> interface_slots; // Only for User Defined variables
const std::vector<uint32_t> builtin_block;
Expand Down Expand Up @@ -529,6 +528,8 @@ struct EntryPoint {
bool has_passthrough{false};
bool has_alpha_to_coverage_variable{false}; // only for Fragment shaders

bool has_physical_storage_buffer_interface{false};

EntryPoint(const Module &module_state, const Instruction &entrypoint_insn, const ImageAccessMap &image_access_map,
const AccessChainVariableMap &access_chain_map, const VariableAccessMap &variable_access_map,
const DebugNameMap &debug_name_map);
Expand Down
Loading

0 comments on commit cdbc347

Please sign in to comment.