spirv-opt: private_to_local_pass does not update OpVariable double pointer

[heroseh]

I have been working on adding support for SPIR-V VariablePointers to my compiler and generate the following

code as so:

_Thread_local uint32_t v;

HCC_COMPUTE(8, 8, 1)
void load_thread_local(HccComputeSV const* const sv, BC const* addrsp(BC) const bc) {
	const uint32_t* addrsp(THREAD) addr = &v;
	uint32_t a = *addr;
	uint32_t b = v;
}

  %v = OpVariable %_ptr_Private_uint Private %uint_0
%load_thread_local = OpFunction %void None %function_type__1561_
 %37 = OpLabel
 %31 = OpVariable %_ptr_Function__ptr_Private_uint Function
 %32 = OpVariable %_ptr_Function_uint Function
 %33 = OpVariable %_ptr_Function_uint Function
       OpStore %31 %v
 %34 = OpLoad %_ptr_Private_uint %31
 %35 = OpLoad %uint %34
       OpStore %32 %35
 %36 = OpLoad %uint %v
       OpStore %33 %36
       OpReturn
       OpFunctionEnd

When this gets optimized through spirv-opt it ends up like this:

%load_thread_local = OpFunction %void None %function_type__1561_
 %37 = OpLabel
  %v = OpVariable %_ptr_Function_uint Function %uint_0
 %31 = OpVariable %_ptr_Function__ptr_Private_uint Function
 %32 = OpVariable %_ptr_Function_uint Function
 %33 = OpVariable %_ptr_Function_uint Function
       OpStore %31 %v
       OpStore %32 %uint_0
       OpStore %33 %uint_0
       OpReturn
       OpFunctionEnd

After running the optimized spirv binary through spirv-val, you get this validation error:

error: line 81: OpStore Pointer <id> '31[%31]'s type does not match Object <id> '47[%v]'s type.
  OpStore %31 %v

In the private_to_local_pass the code does not adjust the types to any OpVariable's which are ptr(FUNCTION, ptr(PRIVATE)) when a variable gets moved from being a private to a local variable.

I have attached two different spirv files where I have got the same bug, along with the source files they were compiled from.

spirvopt_bug_private_to_function_var.tar.gz

I have tried to look around the code and fix the problem, but I don't quite know my way around the codebase just yet :D

It looks like the problem needs to be fixed in here PrivateToLocalPass::UpdateUse

I have tested on the latest code on main

Thank you :)

[s-perron]

If the address of the private variable is stored to a variable, that variable should not be moved to the function storage class. I should remain in the private storage class. The pass does not do enough analysis to know if that the new function scope variable will still be in scope when the stored address is retrieved and used in a memory access.

The solution would be to update IsValidUse to take the operand index as a paramter. Then the use is valid only if the index is the pointer operand (and not the object operand) of an OpStore. I think the store is the only opcode mentioned in the function that requires special handling.

SPIRV-Tools/source/opt/private_to_local_pass.cpp

Lines 156 to 157 in da48bb2

	bool PrivateToLocalPass::IsValidUse(const Instruction* inst) const {
	// The cases in this switch have to match the cases in |UpdateUse|.

[s-perron]

If want to try fixing it ourself, let me know. I can review it for you.

[heroseh]

Yeah that solution makes a lot of sense. Had another look at the code, got a vague idea but not quite sure how I could do that yet :)

[heroseh]

I have had some more time to study the code, I have found exactly what to do. I'll hopefully have some time to make an MR over the weekend, thank you