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: DominatorAnalysis is not invalidated but the CFG is #3635

Closed
Vasniktel opened this issue Aug 1, 2020 · 1 comment · Fixed by #4733
Closed

spirv-opt: DominatorAnalysis is not invalidated but the CFG is #3635

Vasniktel opened this issue Aug 1, 2020 · 1 comment · Fixed by #4733
Assignees

Comments

@Vasniktel
Copy link
Collaborator

Vasniktel commented Aug 1, 2020

MergeReturnPass invalidates CFG but doesn't invalidate dominator trees here

// Make sure the CFG is build here. If we don't then it becomes very hard
// to know which new blocks need to be updated.
context()->BuildInvalidAnalyses(IRContext::kAnalysisCFG);

Dominator trees depend on CFG's pseudo entry and exit blocks. When CFG is invalidated, dominator trees contain dangling pointers. This causes assertion failures later in the program.

The bug was found by inspecting an assertion failure in the attached shader. The failure happens in

iterator tail() {
assert(!insts_.empty());

when one tries to run spirv-opt <attached file> -o out.spv --validate-after-all --if-conversion --merge-return. --if-conversion is used only to initialize dominator trees before --merge-return is executed.

data.zip contains a GLSL shader and its compiled SPIR-V version.

Can be reproduced on b78f4b1.

@Vasniktel
Copy link
Collaborator Author

@alan-baker @s-perron ping

@s-perron s-perron self-assigned this Aug 10, 2020
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Feb 28, 2022
In BuildInvalidAnalyses, we state that it will only rebuild the invalid
analysies.  However, the code will rebuils all analyses that have been
requested, even if they were already valid.

We fix that, and add a couple tests.  The tests cannot check every
analysis because there are no visible differences for all of the
analyses.  We test two of them as representatives.  Note that the new
code is generic, so this should be good enough.

Fixes KhronosGroup#3635
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Mar 1, 2022
In BuildInvalidAnalyses, we state that it will only rebuild the invalid
analyses.  However, the code will rebuild all analyses that have been
requested, even if they were already valid.

We fix that, and add a couple tests.  The tests cannot check every
analysis because not all of the analyses have visible sideeffects of
being rebuilt.  We test two of them as representatives.  Note that the new
code is generic, so this should be good enough.

Fixes KhronosGroup#3635
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.

2 participants