Fix up how OpImageTexelPointer is interpreted.

[s-perron]

The only spot we do anything special with OpImageTexelPointer is when trying to get the base pointer. In that place, we treat it like an OpAccessChain where it manipulates the operand pointer.

When looking at an example that uses it, I think this interpretation is not the best way of thinking about it. Consider the following example,

               OpCapability SampledBuffer
               OpCapability StorageImageExtendedFormats
               OpCapability ImageBuffer
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %2 "min" %gl_GlobalInvocationID
               OpExecutionMode %2 LocalSize 64 1 1
               OpSource HLSL 600
               OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
               OpDecorate %4 DescriptorSet 4
               OpDecorate %4 Binding 70
       %uint = OpTypeInt 32 0
          %6 = OpTypeImage %uint Buffer 0 0 0 2 R32ui
%_ptr_UniformConstant_6 = OpTypePointer UniformConstant %6
%_ptr_Private_6 = OpTypePointer Private %6
       %void = OpTypeVoid
         %10 = OpTypeFunction %void
     %uint_0 = OpConstant %uint 0
     %uint_1 = OpConstant %uint 1
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
%_ptr_Image_uint = OpTypePointer Image %uint
          %4 = OpVariable %_ptr_UniformConstant_6 UniformConstant
         %16 = OpVariable %_ptr_Private_6 Private
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
          %2 = OpFunction %void None %10
         %17 = OpLabel
         %18 = OpLoad %6 %4
               OpStore %16 %18
         %19 = OpImageTexelPointer %_ptr_Image_uint %16 %uint_0 %uint_0
         %20 = OpAtomicIAdd %uint %19 %uint_1 %uint_0 %uint_1
               OpReturn
               OpFunctionEnd

In this case, the atomic add is not updating the local variable. The best way of treating this is that the OpImageTexelPointer is like a load of the input pointer and a manipulation of the value loaded. We need to treat it that way.

In particular, ADCE needs to know treat it like a load so that the store to the local is not removed.

[greg-lunarg]

This code does not look "right" to me.

I think there is something missing in the SPIR-V spec.

Specifically, it is surprising to me that there is not a rule in the spec that requires the Storage Class of the Image in an OpImageTexelPointer to be "Image".

The spec requires the Storage Class of the Result Type pointee to be "Image". Shouldn't the Storage Class of the result's pointee and the referenced pointee match? It is hard to imagine why these should not be required to match. What good is the requirement for the pointer otherwise?

If indeed it is decided that the Image must be of Storage Class "Image", then the code in this issue is invalid and this issue can be closed. I suspect the other issues related to this one can then also be closed as such pointers cannot be interpreted to point at Private or Function scope variables.

[dnovillo]

@greg-lunarg isn't the spec saying that now? In https://www.khronos.org/registry/spir-v/specs/1.2/SPIRV.html#OpImageTexelPointer, it states: "Image must have a type of OpTypePointer with Type OpTypeImage. The Sampled Type of the type of Image must be the same as the Type pointed to by Result Type. The Dim operand of Type cannot be SubpassData."

Which is what this code seems to be doing. I may be reading something wrong, though.

We need to keep %16 live. The OpImageTexelPointer instruction just does a type cast. So, the store into %19, must be considered a store into %16.

@dneto0, could you confirm?

[s-perron]

We discussed with @greg-lunarg. When looking at the code that is currently generated for an OpImageWrite, it looks like the the current model that an OpLoad of an image variable returns a "handle" to the image. It is consistent that an OpImageTexelPointer is treated the way I am dealing with it. Also, alternative solutions would require significant changes to the FE. So we will put merge the PRs associated with this issue.

However, this is still odd. Why would an OpImageTexelPointer take a pointer to an image instead of an image? We need to verify that the semantics I have given to OpImageTexelPointer is correct. @greg-lunarg will open a follow-up issue to discuss this. We can then implement any changes that come out of that discuss.