Skip to content

Commit

Permalink
Fix how array buf refs are accessed
Browse files Browse the repository at this point in the history
  • Loading branch information
@spencer-lunarg
spencer-lunarg committed Nov 8, 2023
1 parent 9a2d895 commit c063574
Show file tree
Hide file tree
Showing 15 changed files with 270 additions and 1,935 deletions.
82 changes: 45 additions & 37 deletions common/output_stream.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <iomanip>
#include <sstream>
#include <string>
#include <unordered_set>
#include <vector>

enum TextLineType {
Expand Down Expand Up @@ -645,6 +646,7 @@ std::string ToStringTypeFlags(SpvReflectTypeFlags type_flags) {
std::stringstream sstream;
PRINT_AND_CLEAR_TYPE_FLAG(sstream, type_flags, ARRAY);
PRINT_AND_CLEAR_TYPE_FLAG(sstream, type_flags, STRUCT);
PRINT_AND_CLEAR_TYPE_FLAG(sstream, type_flags, REF);
PRINT_AND_CLEAR_TYPE_FLAG(sstream, type_flags, EXTERNAL_MASK);
PRINT_AND_CLEAR_TYPE_FLAG(sstream, type_flags, EXTERNAL_BLOCK);
PRINT_AND_CLEAR_TYPE_FLAG(sstream, type_flags, EXTERNAL_SAMPLED_IMAGE);
Expand Down Expand Up @@ -938,15 +940,12 @@ std::string ToStringComponentType(const SpvReflectTypeDescription& type,
return ss.str();
}

// TODO 212 - Formatting for buffer refference needs to be improved
static std::unordered_set<uint32_t> physical_pointer_spirv_id;

void ParseBlockMembersToTextLines(const char* indent, int indent_depth,
bool flatten_cbuffers,
const std::string& parent_name,
uint32_t member_count,
const SpvReflectBlockVariable* p_members,
std::vector<TextLine>* p_text_lines) {
void ParseBlockMembersToTextLines(
const char* indent, int indent_depth, bool flatten_cbuffers,
const std::string& parent_name, uint32_t member_count,
const SpvReflectBlockVariable* p_members,
std::vector<TextLine>* p_text_lines,
std::unordered_set<uint32_t>& physical_pointer_spirv_id) {
const char* t = indent;
for (uint32_t member_index = 0; member_index < member_count; ++member_index) {
indent_depth = flatten_cbuffers ? 2 : indent_depth;
Expand All @@ -961,18 +960,16 @@ void ParseBlockMembersToTextLines(const char* indent, int indent_depth,
// TODO 212 - If a buffer ref has an array of itself, all members are null
continue;
}
if (physical_pointer_spirv_id.count(member.spirv_id)) {
continue;
} else {
physical_pointer_spirv_id.insert(member.spirv_id);
}

bool is_struct =
((member.type_description->type_flags &
static_cast<SpvReflectTypeFlags>(SPV_REFLECT_TYPE_FLAG_STRUCT)) != 0);
bool is_ref =
((member.type_description->type_flags &
static_cast<SpvReflectTypeFlags>(SPV_REFLECT_TYPE_FLAG_REF)) != 0);
bool is_array =
((member.type_description->type_flags &
static_cast<SpvReflectTypeFlags>(SPV_REFLECT_TYPE_FLAG_ARRAY)) != 0);
if (is_struct) {
const std::string name = (member.name == nullptr ? "" : member.name);

Expand All @@ -994,20 +991,37 @@ void ParseBlockMembersToTextLines(const char* indent, int indent_depth,
p_text_lines->push_back(tl);
}

// Members
tl = {};
std::string current_parent_name;
if (flatten_cbuffers) {
current_parent_name =
parent_name.empty() ? name : (parent_name + "." + name);
const bool array_of_structs =
is_array && member.type_description->struct_type_description;
const uint32_t struct_id =
array_of_structs
? member.type_description->struct_type_description->id
: member.type_description->id;

if (physical_pointer_spirv_id.count(struct_id) == 0) {
physical_pointer_spirv_id.insert(member.type_description->id);
if (array_of_structs) {
physical_pointer_spirv_id.insert(
member.type_description->struct_type_description->id);
}

// Members
tl = {};
std::string current_parent_name;
if (flatten_cbuffers) {
current_parent_name =
parent_name.empty() ? name : (parent_name + "." + name);
}
std::vector<TextLine>* p_target_text_line =
flatten_cbuffers ? p_text_lines : &tl.lines;
ParseBlockMembersToTextLines(t, indent_depth + 1, flatten_cbuffers,
current_parent_name, member.member_count,
member.members, p_target_text_line,
physical_pointer_spirv_id);
tl.text_line_flags = TEXT_LINE_TYPE_LINES;
p_text_lines->push_back(tl);
}
std::vector<TextLine>* p_target_text_line =
flatten_cbuffers ? p_text_lines : &tl.lines;
ParseBlockMembersToTextLines(t, indent_depth + 1, flatten_cbuffers,
current_parent_name, member.member_count,
member.members, p_target_text_line);
tl.text_line_flags = TEXT_LINE_TYPE_LINES;
p_text_lines->push_back(tl);
physical_pointer_spirv_id.erase(member.type_description->id);

// End struct
tl = {};
Expand Down Expand Up @@ -1085,6 +1099,7 @@ void ParseBlockMembersToTextLines(const char* indent, int indent_depth,
p_text_lines->push_back(tl);
}
}
// physical_pointer_spirv_id.clear();
}

void ParseBlockVariableToTextLines(const char* indent, bool flatten_cbuffers,
Expand All @@ -1103,9 +1118,10 @@ void ParseBlockVariableToTextLines(const char* indent, bool flatten_cbuffers,

// Members
tl = {};
std::unordered_set<uint32_t> physical_pointer_spirv_id;
ParseBlockMembersToTextLines(indent, 2, flatten_cbuffers, "",
block_var.member_count, block_var.members,
&tl.lines);
&tl.lines, physical_pointer_spirv_id);
tl.text_line_flags = TEXT_LINE_TYPE_LINES;
p_text_lines->push_back(tl);

Expand Down Expand Up @@ -1743,12 +1759,6 @@ void SpvReflectToYaml::WriteTypeDescription(std::ostream& os,
void SpvReflectToYaml::WriteBlockVariable(std::ostream& os,
const SpvReflectBlockVariable& bv,
uint32_t indent_level) {
if (physical_pointer_block_variable_.count(&bv)) {
return;
} else {
physical_pointer_block_variable_.insert(&bv);
}

for (uint32_t i = 0; i < bv.member_count; ++i) {
WriteBlockVariable(os, bv.members[i], indent_level);
}
Expand Down Expand Up @@ -1828,9 +1838,7 @@ void SpvReflectToYaml::WriteBlockVariable(std::ostream& os,
os << t1 << "members:" << std::endl;
for (uint32_t i = 0; i < bv.member_count; ++i) {
auto itor = block_variable_to_index_.find(&bv.members[i]);
if (itor == block_variable_to_index_.end()) {
continue;
}
assert(itor != block_variable_to_index_.end());
os << t2 << "- *bv" << itor->second << std::endl;
}
if (verbosity_ >= 1) {
Expand Down
3 changes: 0 additions & 3 deletions common/output_stream.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include <map>
#include <ostream>
#include <string>
#include <unordered_set>

#include "spirv_reflect.h"

Expand Down Expand Up @@ -67,8 +66,6 @@ class SpvReflectToYaml {
uint32_t verbosity_ = 0;
std::map<const SpvReflectTypeDescription*, uint32_t> type_description_to_index_;
std::map<const SpvReflectBlockVariable*, uint32_t> block_variable_to_index_;
std::unordered_set<const SpvReflectBlockVariable*>
physical_pointer_block_variable_;
std::map<const SpvReflectDescriptorBinding*, uint32_t> descriptor_binding_to_index_;
std::map<const SpvReflectInterfaceVariable*, uint32_t> interface_variable_to_index_;
};
Expand Down
120 changes: 73 additions & 47 deletions spirv_reflect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ typedef struct SpvReflectPrvParser {
uint32_t descriptor_count;
uint32_t push_constant_count;

uint32_t physical_pointer_check[MAX_RECURSIVE_PHYSICAL_POINTER_CHECK];
SpvReflectTypeDescription* physical_pointer_check[MAX_RECURSIVE_PHYSICAL_POINTER_CHECK];
uint32_t physical_pointer_count;

SpvReflectPrvPhysicalPointerStruct* physical_pointer_structs;
Expand Down Expand Up @@ -1970,15 +1970,18 @@ static SpvReflectResult ParseType(
if (p_type->storage_class == SpvStorageClassPhysicalStorageBuffer) {
// Need to make sure we haven't started an infinite recursive loop
for (uint32_t i = 0; i < p_parser->physical_pointer_count; i++) {
if (p_type->id == p_parser->physical_pointer_check[i]) {
if (p_type->id == p_parser->physical_pointer_check[i]->id) {
found_recursion = true;
break; // still need to fill in p_type values
memcpy(p_type, p_parser->physical_pointer_check[i],
sizeof(SpvReflectTypeDescription));
p_type->copied = 1;
return SPV_REFLECT_RESULT_SUCCESS;
}
}
if (!found_recursion) {
p_parser->physical_pointer_struct_count++;
p_parser->physical_pointer_check[p_parser->physical_pointer_count] =
p_type->id;
p_type;
p_parser->physical_pointer_count++;
if (p_parser->physical_pointer_count >=
MAX_RECURSIVE_PHYSICAL_POINTER_CHECK) {
Expand Down Expand Up @@ -2466,41 +2469,54 @@ static SpvReflectResult ParseDescriptorBlockVariable(
SpvReflectTypeDescription* p_member_ptr_type = 0;
bool found_recursion = false;

if (p_member_type->op == SpvOpTypePointer) {
if (p_member_type->storage_class ==
SpvStorageClassPhysicalStorageBuffer) {
// Need to make sure we haven't started an infinite recursive loop
for (uint32_t i = 0; i < p_parser->physical_pointer_count; i++) {
if (p_member_type->id == p_parser->physical_pointer_check[i]) {
found_recursion = true;
break; // still need to fill in p_member_type values
}
if (p_member_type->storage_class ==
SpvStorageClassPhysicalStorageBuffer &&
(p_member_type->type_flags & SPV_REFLECT_TYPE_FLAG_REF)) {
// Remember the original type
p_member_ptr_type = p_member_type;

// strip array
if (p_member_type->op == SpvOpTypeArray) {
SpvReflectPrvNode* p_node = FindNode(p_parser, p_member_type->id);
if (p_node == NULL) {
return SPV_REFLECT_RESULT_ERROR_SPIRV_INVALID_ID_REFERENCE;
}
if (!found_recursion) {
uint32_t struct_id = FindType(p_module, p_member_type->id)
->struct_type_description->id;
p_parser
->physical_pointer_structs[p_parser
->physical_pointer_struct_count]
.struct_id = struct_id;
p_parser
->physical_pointer_structs[p_parser
->physical_pointer_struct_count]
.p_var = p_member_var;
p_parser->physical_pointer_struct_count++;
uint32_t element_type_id = p_node->array_traits.element_type_id;
p_member_type = FindType(p_module, element_type_id);
if (p_member_type == NULL) {
return SPV_REFLECT_RESULT_ERROR_SPIRV_INVALID_ID_REFERENCE;
}
}

p_parser->physical_pointer_check[p_parser->physical_pointer_count] =
p_member_type->id;
p_parser->physical_pointer_count++;
if (p_parser->physical_pointer_count >=
MAX_RECURSIVE_PHYSICAL_POINTER_CHECK) {
return SPV_REFLECT_RESULT_ERROR_SPIRV_MAX_RECURSIVE_EXCEEDED;
}
// Need to make sure we haven't started an infinite recursive loop
for (uint32_t i = 0; i < p_parser->physical_pointer_count; i++) {
if (p_member_type->id == p_parser->physical_pointer_check[i]->id) {
found_recursion = true;
break; // still need to fill in p_member_type values
}
}
if (!found_recursion) {
uint32_t struct_id = FindType(p_module, p_member_type->id)
->struct_type_description->id;
p_parser
->physical_pointer_structs[p_parser
->physical_pointer_struct_count]
.struct_id = struct_id;
p_parser
->physical_pointer_structs[p_parser
->physical_pointer_struct_count]
.p_var = p_member_var;
p_parser->physical_pointer_struct_count++;

p_parser->physical_pointer_check[p_parser->physical_pointer_count] =
p_member_type;
p_parser->physical_pointer_count++;
if (p_parser->physical_pointer_count >=
MAX_RECURSIVE_PHYSICAL_POINTER_CHECK) {
return SPV_REFLECT_RESULT_ERROR_SPIRV_MAX_RECURSIVE_EXCEEDED;
}
}

// Remember the original type
p_member_ptr_type = p_member_type;
SpvReflectPrvNode* p_member_type_node =
FindNode(p_parser, p_member_type->id);
if (IsNull(p_member_type_node)) {
Expand Down Expand Up @@ -2572,6 +2588,16 @@ static SpvReflectResult ParseDescriptorBlockVariable(
return SPV_REFLECT_RESULT_SUCCESS;
}

static uint32_t GetPhysicalPointerStructSize(SpvReflectPrvParser* p_parser,
uint32_t id) {
for (uint32_t i = 0; i < p_parser->physical_pointer_struct_count; i++) {
if (p_parser->physical_pointer_structs[i].struct_id == id) {
return p_parser->physical_pointer_structs[i].p_var->size;
}
}
return 0;
}

static SpvReflectResult ParseDescriptorBlockVariableSizes(
SpvReflectPrvParser* p_parser,
SpvReflectShaderModule* p_module,
Expand Down Expand Up @@ -2643,9 +2669,16 @@ static SpvReflectResult ParseDescriptorBlockVariableSizes(
// If array of structs, parse members first...
bool is_struct = (p_member_type->type_flags & SPV_REFLECT_TYPE_FLAG_STRUCT) == SPV_REFLECT_TYPE_FLAG_STRUCT;
if (is_struct) {
SpvReflectResult result = ParseDescriptorBlockVariableSizes(p_parser, p_module, false, true, is_parent_rta, p_member_var);
if (result != SPV_REFLECT_RESULT_SUCCESS) {
return result;
if (p_member_var->flags &
SPV_REFLECT_VARIABLE_FLAGS_PHYSICAL_POINTER_COPY) {
p_member_var->size =
GetPhysicalPointerStructSize(p_parser, p_member_type->id);
} else {
SpvReflectResult result = ParseDescriptorBlockVariableSizes(
p_parser, p_module, false, true, is_parent_rta, p_member_var);
if (result != SPV_REFLECT_RESULT_SUCCESS) {
return result;
}
}
}
// ...then array
Expand Down Expand Up @@ -2686,15 +2719,8 @@ static SpvReflectResult ParseDescriptorBlockVariableSizes(
case SpvOpTypeStruct: {
if (p_member_var->flags &
SPV_REFLECT_VARIABLE_FLAGS_PHYSICAL_POINTER_COPY) {
// if copy, apply size values
for (uint32_t i = 0; i < p_parser->physical_pointer_struct_count;
i++) {
if (p_parser->physical_pointer_structs[i].struct_id ==
p_member_type->id) {
p_member_var->size =
p_parser->physical_pointer_structs[i].p_var->size;
}
}
p_member_var->size =
GetPhysicalPointerStructSize(p_parser, p_member_type->id);
} else {
SpvReflectResult result = ParseDescriptorBlockVariableSizes(
p_parser, p_module, false, is_parent_aos, is_parent_rta,
Expand Down Expand Up @@ -4251,7 +4277,7 @@ SpvReflectResult spvReflectGetShaderModule(

static void SafeFreeTypes(SpvReflectTypeDescription* p_type)
{
if (IsNull(p_type)) {
if (IsNull(p_type) || p_type->copied) {
return;
}

Expand Down
4 changes: 4 additions & 0 deletions spirv_reflect.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,10 @@ typedef struct SpvReflectTypeDescription {
// this gives access to the OpTypeStruct
struct SpvReflectTypeDescription* struct_type_description;

// Some pointers to SpvReflectTypeDescription are really
// just copies of another reference to the same OpType
uint32_t copied;

// @deprecated use struct_type_description instead
uint32_t member_count;
// @deprecated use struct_type_description instead
Expand Down
Loading

0 comments on commit c063574

Please sign in to comment.