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.

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

spirv-opt: OpAccessChain without indices is not supported #3643

Closed
Vasniktel opened this issue Aug 3, 2020 · 5 comments · Fixed by #3678
Closed

spirv-opt: OpAccessChain without indices is not supported #3643

Vasniktel opened this issue Aug 3, 2020 · 5 comments · Fixed by #3678
Assignees

Comments

@Vasniktel
Copy link
Collaborator

The following SPIR-V passes spirv-val's checks

               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %4 "main"
               OpExecutionMode %4 OriginUpperLeft
               OpSource ESSL 310
          %2 = OpTypeVoid
          %3 = OpTypeFunction %2
          %6 = OpTypeBool
          %7 = OpTypePointer Function %6
          %4 = OpFunction %2 None %3
          %5 = OpLabel
         %10 = OpVariable %7 Function
         %11 = OpAccessChain %7 %10
               OpReturn
               OpFunctionEnd

In this code, the Base operand of the OpAccessChain is not a pointer to a composite. Also, indices operands are missing. The same issue arises with OpInBoundsAccessChain.

@Vasniktel Vasniktel changed the title spirv-val: OpAccessChain without indices are permitted spirv-val: OpAccessChain without indices is permitted Aug 3, 2020
@Vasniktel
Copy link
Collaborator Author

@alan-baker @s-perron ping

@s-perron
Copy link
Collaborator

s-perron commented Aug 4, 2020

@alan-baker Where did the spec come done on the legality of this? I remember we had discussions about it because some optimizations assume at least one index. However, I forget the result.

@alan-baker
Copy link
Contributor

It was left as valid because there are a reasonable number of code generators that generate this case.

@Vasniktel
Copy link
Collaborator Author

spirv-opt doesn't support this case, though. This SPIR-V

               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %4 "main"
               OpExecutionMode %4 OriginUpperLeft
               OpSource ESSL 310
          %2 = OpTypeVoid
          %3 = OpTypeFunction %2
          %6 = OpTypeBool
          %7 = OpTypePointer Function %6
          %4 = OpFunction %2 None %3
          %5 = OpLabel
         %10 = OpVariable %7 Function
         %11 = OpAccessChain %7 %10
         %12 = OpLoad %6 %11
               OpReturn
               OpFunctionEnd

produces an error

error: line 16: Expected at least one index to OpCompositeExtract, zero found
%12 = OpCompositeExtract %bool %13

error: line 0: Validation failed after pass convert-local-access-chains

when executed with the following command spirv-opt <attached file> -o out.spv --validate-after-all --convert-local-access-chains. This can be reproduced on 28f32ca.

Compiled shader is in the attached file .

@Vasniktel Vasniktel changed the title spirv-val: OpAccessChain without indices is permitted spirv-opt: OpAccessChain without indices is not permitted Aug 5, 2020
@Vasniktel Vasniktel changed the title spirv-opt: OpAccessChain without indices is not permitted spirv-opt: OpAccessChain without indices is not supported Aug 5, 2020
@paulthomson
Copy link
Contributor

In an internal discussion, it was suggested that the pointer operand must point to a composite (as implied by the spec). Thus, this can be regarded as a spirv-val bug.

And 0 indices is fine, so this can be classed as a spirv-opt bug.

@s-perron s-perron self-assigned this Aug 10, 2020
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Aug 10, 2020
s-perron added a commit that referenced this issue Aug 11, 2020
* Handle no index access chain in local access chain convert

Fixes #3643
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this issue Aug 19, 2020
…oup#3678)

* Handle no index access chain in local access chain convert

Fixes KhronosGroup#3643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants