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: Add const folding for CompositeInsert #4943

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

sjfricke
Copy link
Contributor

Currently doing a OpSpecConstantOp %x CompositeInsert would segfault

This change injects the extra needed OpConstantComposite in order to properly handle folding CompositeInsert

@sjfricke
Copy link
Contributor Author

sjfricke commented Oct 5, 2022

@dnovillo - can this get a CI dispatch, this is preventing a real world app from using the Vulkan Validation layers (since it uses this pass and the shader had a CompositeInsert spec op)

source/opt/const_folding_rules.cpp Outdated Show resolved Hide resolved
source/opt/const_folding_rules.cpp Outdated Show resolved Hide resolved
for (size_t i = chain.size(); i > 0; i--) {
// Need to insert any previous instruction into the module first
// TODO - Is there a better way to find this position
Module::inst_iterator* pos = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also insert at the top of the constants declared in the module, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I guess I was going for keeping the instructions next to each other, but I also like the idea of making the code simpler and just putting at the top (since I should have an easy reference to its iterator)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried this approach and the issue currently there is only an iterator to the Types which ends up inserting it above where the types are declared. Updated comment and think this is the best way currently to find the instruction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

friendly ping

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry! LGTM.

dnovillo
dnovillo previously approved these changes Oct 26, 2022
dnovillo
dnovillo previously approved these changes Nov 7, 2022
@sjfricke sjfricke force-pushed the sjfricke-stackedop2 branch from 1956d45 to 9262600 Compare November 8, 2022 02:30
spencer-lunarg
spencer-lunarg previously approved these changes Nov 8, 2022
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 this pull request may close these issues.

4 participants