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

crash in --eliminate-local-multi-store #826

Closed
dneto0 opened this issue Sep 19, 2017 · 5 comments
Closed

crash in --eliminate-local-multi-store #826

dneto0 opened this issue Sep 19, 2017 · 5 comments

Comments

@dneto0
Copy link
Collaborator

dneto0 commented Sep 19, 2017

Example from Vulkan CTS, reduces to this:

               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %1 "main"
          %2 = OpTypeBool
          %3 = OpConstantTrue %2
          %4 = OpTypeVoid
          %5 = OpTypeFunction %4
          %6 = OpTypeFloat 32
          %7 = OpTypePointer Function %6
          %8 = OpConstant %6 0
          %9 = OpConstant %6 1
          %1 = OpFunction %4 None %5
         %10 = OpLabel
         %11 = OpVariable %7 Function %8
         %12 = OpVariable %7 Function
         %13 = OpLoad %6 %11
               OpBranch %14
         %14 = OpLabel
         %15 = OpPhi %6 %13 %10 %16 %14
         %16 = OpFAdd %6 %15 %9
               OpLoopMerge %17 %14 None
               OpBranchConditional %3 %14 %17
         %17 = OpLabel
               OpStore %12 %15
               OpReturn
               OpFunctionEnd

The problem appears in processing %16, in the PatchPhis method.

@dneto0
Copy link
Collaborator Author

dneto0 commented Sep 19, 2017

Cc @greg-lunarg

@dneto0
Copy link
Collaborator Author

dneto0 commented Sep 19, 2017

The live var set is empty when processing the self-loop block with id 14. And the pass crashes when processing the (16, 14) pair at the end of the OpPhi with id 15.

@dneto0
Copy link
Collaborator Author

dneto0 commented Sep 19, 2017

I have a fix. Making a test for it.

@greg-lunarg
Copy link
Contributor

Yeah, the code assumes all phi's need to be patched, in actuality, only phi's generated by the elim-multi-store pass need to patched. Is that your fix?

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Sep 20, 2017
There can already be OpPhi instructions in a loop header that
are unrelated to the optimization.  We should not be patching those.

Fixes KhronosGroup#826
@dneto0
Copy link
Collaborator Author

dneto0 commented Sep 20, 2017

Yeah, the code assumes all phi's need to be patched, in actuality, only phi's generated by the elim-multi-store pass need to patched. Is that your fix?

Yes. Please take a look at #828

antiagainst pushed a commit that referenced this issue Sep 21, 2017
There can already be OpPhi instructions in a loop header that
are unrelated to the optimization.  We should not be patching those.

Fixes #826
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this issue Sep 21, 2017
There can already be OpPhi instructions in a loop header that
are unrelated to the optimization.  We should not be patching those.

Fixes KhronosGroup#826
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

No branches or pull requests

2 participants