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.

Sign up for GitHub

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

Jump to bottom

spirv-fuzz: Create a helper in fuzzerutil to reuse function type #3572

Merged
merged 4 commits into from Jul 23, 2020
Merged

spirv-fuzz: Create a helper in fuzzerutil to reuse function type #3572

merged 4 commits into from Jul 23, 2020

Conversation

Vasniktel
Copy link
Collaborator

Fixes #3534.

@afd afd added the kokoro:run label Jul 22, 2020
afd
afd suggested changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Looks good! Some changes requested.

source/fuzz/fuzzer_util.cpp Outdated Show resolved Hide resolved
source/fuzz/fuzzer_util.h Outdated Show resolved Hide resolved
source/fuzz/fuzzer_util.cpp Outdated Show resolved Hide resolved
@Vasniktel Vasniktel requested a review from afd July 22, 2020 13:02
@afd afd added the kokoro:run label Jul 22, 2020
afd
afd reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

The changes look good. I left a couple of comments on variable naming that I recommend making but don't insist on.

There's one thing I don't like about this: suppose that f's current type has only 1 user, but the new type for f already exists. In that case we add the new type to the module and change f so it has that type, but we leave the old type in the module even though it has no users.

I feel like it would be more natural to remove the old type in that case. What do you think @Vasniktel ? Perhaps I am over-thinking it - it would be unlikely to arise.

source/fuzz/transformation_permute_function_parameters.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_parameter_with_global.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_params_with_struct.cpp Outdated Show resolved Hide resolved
@Vasniktel Vasniktel requested a review from afd July 22, 2020 14:53
afd
afd approved these changes Jul 23, 2020
View reviewed changes
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

LGTM!

@afd afd added the kokoro:run label Jul 23, 2020
@afd afd merged commit 1aaf5c6 into KhronosGroup:master Jul 23, 2020
@Vasniktel Vasniktel deleted the reuse_function_type branch July 24, 2020 08:50
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this pull request Aug 19, 2020
@Vasniktel @dnovillo
spirv-fuzz: Create a helper in fuzzerutil to reuse function type (Khr…
2d6dc4c 
…onosGroup#3572)

Fixes KhronosGroup#3534.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afd afd afd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spirv-fuzz: Refactor boilerplate in TransformationReplaceParameter*
4 participants
@Vasniktel @afd @paulthomson @kokoro-team