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

Correctly report liveness of variables passed as function parameters #267

Merged
merged 3 commits into from
Jul 12, 2024
Merged

Correctly report liveness of variables passed as function parameters #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
541ee47
関数の引数に渡された変数のアクセス状況を解析するように修正
Jun 26, 2024
5fac308
Update golden YAML
daniel-story Jun 26, 2024
16ac489
Add function parameter access test
daniel-story Jun 26, 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
95 changes: 88 additions & 7 deletions spirv_reflect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,14 @@ typedef struct SpvReflectPrvAccessedVariable {
SpvReflectPrvNode* p_node;
uint32_t result_id;
uint32_t variable_ptr;
uint32_t function_id;
uint32_t function_parameter_index;
} SpvReflectPrvAccessedVariable;

typedef struct SpvReflectPrvFunction {
uint32_t id;
uint32_t parameter_count;
uint32_t* parameters;
uint32_t callee_count;
uint32_t* callees;
struct SpvReflectPrvFunction** callee_ptrs;
Expand Down Expand Up @@ -642,6 +646,7 @@ static void DestroyParser(SpvReflectPrvParser* p_parser) {

// Free functions
for (size_t i = 0; i < p_parser->function_count; ++i) {
SafeFree(p_parser->functions[i].parameters);
SafeFree(p_parser->functions[i].callees);
SafeFree(p_parser->functions[i].callee_ptrs);
SafeFree(p_parser->functions[i].accessed_variables);
Expand Down Expand Up @@ -1095,6 +1100,7 @@ static SpvReflectResult ParseFunction(SpvReflectPrvParser* p_parser, SpvReflectP
size_t first_label_index) {
p_func->id = p_func_node->result_id;

p_func->parameter_count = 0;
p_func->callee_count = 0;
p_func->accessed_variable_count = 0;

Expand All @@ -1105,7 +1111,11 @@ static SpvReflectResult ParseFunction(SpvReflectPrvParser* p_parser, SpvReflectP
break;
}
switch (p_node->op) {
case SpvOpFunctionParameter: {
++(p_func->parameter_count);
} break;
case SpvOpFunctionCall: {
p_func->accessed_variable_count += p_node->word_count - 4;
++(p_func->callee_count);
} break;
case SpvOpLoad:
Expand All @@ -1128,6 +1138,13 @@ static SpvReflectResult ParseFunction(SpvReflectPrvParser* p_parser, SpvReflectP
}
}

if (p_func->parameter_count > 0) {
p_func->parameters = (uint32_t*)calloc(p_func->parameter_count, sizeof(*(p_func->parameters)));
if (IsNull(p_func->parameters)) {
return SPV_REFLECT_RESULT_ERROR_ALLOC_FAILED;
}
}

if (p_func->callee_count > 0) {
p_func->callees = (uint32_t*)calloc(p_func->callee_count, sizeof(*(p_func->callees)));
if (IsNull(p_func->callees)) {
Expand All @@ -1143,6 +1160,7 @@ static SpvReflectResult ParseFunction(SpvReflectPrvParser* p_parser, SpvReflectP
}
}

p_func->parameter_count = 0;
p_func->callee_count = 0;
p_func->accessed_variable_count = 0;
// Now have allocation, fill in values
Expand All @@ -1152,8 +1170,25 @@ static SpvReflectResult ParseFunction(SpvReflectPrvParser* p_parser, SpvReflectP
break;
}
switch (p_node->op) {
case SpvOpFunctionParameter: {
CHECKED_READU32(p_parser, p_node->word_offset + 2, p_func->parameters[p_func->parameter_count]);
(++p_func->parameter_count);
} break;
case SpvOpFunctionCall: {
CHECKED_READU32(p_parser, p_node->word_offset + 3, p_func->callees[p_func->callee_count]);
const uint32_t result_index = p_node->word_offset + 2;
for (uint32_t j = 0, parameter_count = p_node->word_count - 4; j < parameter_count; j++) {
const uint32_t ptr_index = p_node->word_offset + 4 + j;
SpvReflectPrvAccessedVariable* access_ptr = &p_func->accessed_variables[p_func->accessed_variable_count];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does access_ptr indicate that it's accessing a pointer or is the _ptr suffix to indicate that this is variable is a pointer? If it's the latter, then it should be p_access instead.

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 line was copied and pasted from the SpvOpLoad..SpvOpImageTexelPointer case below (see line 1204). I can change it on this line, but perhaps it's better to fix both lines at once in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, apologies I didn't see that. You're right, that should be handled in a separate PR, not critical.


access_ptr->p_node = p_node;
// Need to track Result ID as not sure there has been any memory access through here yet
CHECKED_READU32(p_parser, result_index, access_ptr->result_id);
CHECKED_READU32(p_parser, ptr_index, access_ptr->variable_ptr);
access_ptr->function_id = p_func->callees[p_func->callee_count];
access_ptr->function_parameter_index = j;
(++p_func->accessed_variable_count);
}
(++p_func->callee_count);
} break;
case SpvOpLoad:
Expand Down Expand Up @@ -1238,14 +1273,12 @@ static SpvReflectResult ParseFunctions(SpvReflectPrvParser* p_parser) {

// Skip over function declarations that aren't definitions
bool func_definition = false;
// Intentionally reuse i to avoid iterating over these nodes more than
// once
for (; i < p_parser->node_count; ++i) {
if (p_parser->nodes[i].op == SpvOpLabel) {
for (size_t j = i; j < p_parser->node_count; ++j) {
if (p_parser->nodes[j].op == SpvOpLabel) {
func_definition = true;
break;
}
if (p_parser->nodes[i].op == SpvOpFunctionEnd) {
if (p_parser->nodes[j].op == SpvOpFunctionEnd) {
break;
}
}
Expand Down Expand Up @@ -3439,6 +3472,44 @@ static SpvReflectResult ParseByteAddressBuffer(SpvReflectPrvParser* p_parser, Sp
return SPV_REFLECT_RESULT_SUCCESS;
}

static SpvReflectResult ParseFunctionParameterAccess(SpvReflectPrvParser* p_parser, uint32_t callee_function_id,
uint32_t function_parameter_index, uint32_t* p_accessed) {
SpvReflectPrvFunction* p_func = NULL;
for (size_t i = 0; i < p_parser->function_count; ++i) {
if (p_parser->functions[i].id == callee_function_id) {
p_func = &(p_parser->functions[i]);
break;
}
}
if (p_func == NULL) {
return SPV_REFLECT_RESULT_ERROR_SPIRV_INVALID_ID_REFERENCE;
}

assert(function_parameter_index < p_func->parameter_count);

for (size_t i = 0; i < p_func->accessed_variable_count; ++i) {
if (p_func->parameters[function_parameter_index] == p_func->accessed_variables[i].variable_ptr) {
SpvReflectPrvAccessedVariable* p_var = &p_func->accessed_variables[i];
if (p_var->function_id > 0) {
SpvReflectResult result =
ParseFunctionParameterAccess(p_parser, p_var->function_id, p_var->function_parameter_index, p_accessed);
if (result != SPV_REFLECT_RESULT_SUCCESS) {
return result;
}
} else {
*p_accessed = 1;
}
// Early out as soon as p_accessed is true
if (*p_accessed) {
return SPV_REFLECT_RESULT_SUCCESS;
}
}
}

*p_accessed = 0;
return SPV_REFLECT_RESULT_SUCCESS;
}

static SpvReflectResult ParseStaticallyUsedResources(SpvReflectPrvParser* p_parser, SpvReflectShaderModule* p_module,
SpvReflectEntryPoint* p_entry, size_t uniform_count, uint32_t* uniforms,
size_t push_constant_count, uint32_t* push_constants) {
Expand Down Expand Up @@ -3535,8 +3606,18 @@ static SpvReflectResult ParseStaticallyUsedResources(SpvReflectPrvParser* p_pars
uint32_t byte_address_buffer_offset_count = 0;

for (uint32_t j = 0; j < used_acessed_count; j++) {
if (p_used_accesses[j].variable_ptr == p_binding->spirv_id) {
p_binding->accessed = 1;
SpvReflectPrvAccessedVariable* p_var = &p_used_accesses[j];
if (p_var->variable_ptr == p_binding->spirv_id) {
if (p_var->function_id > 0) {
result =
ParseFunctionParameterAccess(p_parser, p_var->function_id, p_var->function_parameter_index, &p_binding->accessed);
if (result != SPV_REFLECT_RESULT_SUCCESS) {
SafeFree(p_used_accesses);
return result;
}
} else {
p_binding->accessed = 1;
}

if (HasByteAddressBufferOffset(p_used_accesses[j].p_node, p_binding)) {
byte_address_buffer_offset_count++;
Expand Down
51 changes: 51 additions & 0 deletions tests/issues/102/function_parameter_access.glsl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
glslangValidator -V -S frag -g0 --ku -o function_parameter_access.spv function_parameter_access.glsl
*/

#version 450

layout(binding = 0) uniform sampler Sampler;
layout(binding = 1) uniform texture2D Texture;
layout(binding = 2) uniform sampler2D Sampler2D;

layout(binding = 3) uniform sampler NeverAccessedSampler;
layout(binding = 4) uniform texture2D NeverAccessedTexture;
layout(binding = 5) uniform sampler2D NeverAccessedSampler2D;

layout(location = 0) out vec4 color;

vec4 access_sampler_and_texture(texture2D t, sampler s, vec2 coord)
{
vec4 ret = texture(sampler2D(t, s), coord);
return vec4(5.0) * ret;
}

vec4 access_combined_sampler(sampler2D s)
{
vec2 coord = vec2(0.5, 0.5);
vec4 ret = texture(s, coord);
return vec4(1.0, 2.0, 3.0, 1.0) * ret;
}

vec4 call_access_functions(texture2D t, sampler s)
{
return access_combined_sampler(Sampler2D) + access_sampler_and_texture(t, s, vec2(0.25, 0.75));
}

vec4 never_called(texture2D t, sampler s, float u, float v)
{
vec4 ret = texture(sampler2D(t, s), vec2(u, v));
return vec4(-3.0) * ret;
}

vec4 never_called_2(vec2 coord)
{
vec4 ret = texture(sampler2D(NeverAccessedTexture, NeverAccessedSampler), coord);
ret *= texture(NeverAccessedSampler2D, coord);
return ret;
}

void main()
{
color = vec4(-1.0) * call_access_functions(Texture, Sampler);
}
Binary file added tests/issues/102/function_parameter_access.spv
View file
Open in desktop
Binary file not shown.
Loading
Loading