Skip to content

Add fix storage class code.#2434

Merged
s-perron merged 2 commits intoKhronosGroup:masterfrom
s-perron:storage_class
Apr 5, 2019
Merged

Add fix storage class code.#2434
s-perron merged 2 commits intoKhronosGroup:masterfrom
s-perron:storage_class

Conversation

@s-perron
Copy link
Copy Markdown
Collaborator

@s-perron s-perron commented Mar 8, 2019

This pass tries to fix validation error due to a mismatch of storage classes
in instructions. There is no guarantee that all such error will be fixed,
and it is possible that in fixing these errors, it could lead to other
errors.

Fixes #2430.

@s-perron s-perron self-assigned this Mar 8, 2019
@jaebaek jaebaek self-requested a review March 12, 2019 15:41
This pass tries to fix validation error due to a mismatch of storage classes
in instructions.  There is no guarantee that all such error will be fixed,
and it is possible that in fixing these errors, it could lead to other
errors.

Fixes KhronosGroup#2430.
@s-perron s-perron changed the title [WIP]: Add fix storage class code. Add fix storage class code. Apr 3, 2019
@s-perron s-perron requested review from dnovillo and jaebaek April 3, 2019 18:51
}
}
});
return modified ? Status::SuccessWithChange : Status::SuccessWithoutChange;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should there be a warning/error/failure if a storage class mismatch was detected but could not be fixed? Or does this pass not detect this case?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This pass does not check if it fails to fix something. It would require more code to check if it fails. Like comparing function parameters for type mismatches or looking at every operand of an OpPhi to make sure the types all match. This is code that is not needed for the pass, and already exists in the validator.

Running the validator will identify anything that has not been fixed. I did not feel the need to make it part of this pass.

inst, [&uses](Instruction* use) { uses.push_back(use); });
for (Instruction* use : uses) {
modified |= PropagateStorageClass(
use, static_cast<SpvStorageClass>(inst->GetSingleWordInOperand(0)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dunno if this can happen in an instruction. What if there are 2 different storage classes in the instruction uses?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This can happen for OpPhi instructions if the type are legitimately different storage types. It will be wrong no matter what we do because the input operands have to match. We will end up picking one of them arbitrarily. Since it cannot be fixed, the choice does not matter. The same for an OpSelect.

All other instructions that we try to rewrite only have a single pointer operand, and the result type has to have the same storage class.

@s-perron s-perron requested a review from dnovillo April 4, 2019 14:10
@s-perron s-perron merged commit 3a0bc9e into KhronosGroup:master Apr 5, 2019
@s-perron s-perron deleted the storage_class branch April 5, 2019 17:12
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.

Legalization of Workgroup function parameter

4 participants