spirv-fuzz: Add support for OpConvert to TransformationEquationInstruction by Vasniktel · Pull Request #3472 · KhronosGroup/SPIRV-Tools

Vasniktel · 2020-06-28T09:05:47Z

Part of #3440.

afd

There's too much going on in this PR for my liking. It all looks like good stuff, but it's hard to review in one go.

It seems like three things are going on:

(1) You have added helpers to check for and to create various types in fuzzer util, and refactored existing code in fuzzer pass to use these. That's good and it can be its own PR (even if it doesn't yet include the code that uses the new functions).

(2) You have changed the transformation that adds equations so that it uses multiple fresh ids. This I'm less sure about. It's not clear to me why it needs multiple fresh ids. Seeing a PR with this change on its own would help me to understand this better. To me, an equation should define a single new id in terms of multiple existing ids; I'm unsure why it would need multiple fresh ids - but happy to discuss.

(3) You have made changes to support equations to convert back and forth between float and int, which looks nice.

In summary:

(1) should definitely be its own PR

(2) and (3) could perhaps be in the same PR if (2) is really needed.

Vasniktel · 2020-06-30T14:46:54Z

(2) is not technically required for this transformation to work but I've recalled from your previous review (#3447 (comment)) that we need to create types on demand. We might need more than one fresh id if, for example, we are inserting OpConvertSToF instruction and there is no suitable result type in the module. In that case, we might need to create a new OpTypeFloat instruction and, possibly, an OpTypeVector of float components.
The return type of other instructions, supported by this transformation (e.g. OpIAdd etc), is equal to the return type of their operands which implies that it always exists in the module.

Vasniktel · 2020-07-01T08:48:36Z

Added a PR for fuzzerutil::FindOrCreate* #3479.

afd

This looks good!

However, I think there's a nice opportunity to exploit the fact that SPIR-V is very relaxed about sign. In particular I believe ConvertFToS can store its result in an unsigned type, and ConvertSToF can operate on an unsigned type.

source/fuzz/protobufs/spvtoolsfuzz.proto

source/fuzz/transformation_context.cpp

source/fuzz/transformation_equation_instruction.cpp

source/fuzz/transformation_equation_instruction.h

test/fuzz/transformation_equation_instruction_test.cpp

afd

Looks very good - just a few small things that need addressing.

In the management of equation facts it seems like you combine Float-to-Int with Int-to-Float to get synonyms, but not the other way around; I guess it should go both ways.

source/fuzz/fuzzer_pass_add_equation_instructions.cpp

source/fuzz/transformation_context.cpp

source/fuzz/fact_manager.cpp

afd

Changes look good except I'm a bit confused by the lack of symmetry between the new code you've added to work out new synonym facts and the code there for the opposite direction - can you check that they indeed need to be different and comment if it's really the case?

source/fuzz/fact_manager.cpp

Vasniktel · 2020-07-12T09:00:35Z

Converted to draft since some functionality needs adjustment.

afd

These changes look good. In general I would prefer there to be a bit less in a PR as smaller PRs are easier to review with confidence - e.g., this one contains some refactorings that appear to be independent from the main content of the PR, and arguably we could have separated the contribution that adds the new kinds of equation instructions from the changes that figure out corollaries. But from what I could tell the changes all work well together.

afd · 2020-07-14T20:44:43Z

@Vasniktel Bots failing due to:

/tmpfs/src/github/SPIRV-Tools/source/fuzz/fact_manager.cpp:750:76: error: unused parameter 'dd2' [-Werror,-Wunused-parameter]
    const protobufs::DataDescriptor& dd1, const protobufs::DataDescriptor& dd2,

afd

Some changes requested having reviewed again after a bot failure.

source/fuzz/fact_manager.cpp

afd

LGTM.

…ction (KhronosGroup#3472) Part of KhronosGroup#3440.

Roll third_party/glslang/ b481744..9eef54b (4 commits) KhronosGroup/glslang@b481744...9eef54b $ git log b481744..9eef54b --date=short --no-merges --format='%ad %ae %s' 2020-07-17 bclayton Update license-checker.cfg with simplified rules 2020-07-17 bclayton Add new rules for update of license-checker 2020-07-15 cepheus GLSL/SPV: Propagaet precision qualifier from function to return value. 2020-07-14 bclayton runtests: Check error codes, set LD_LIBRARY_PATH Created with: roll-dep third_party/glslang Roll third_party/googletest/ 70b90929b..a781fe29b (3 commits) google/googletest@70b9092...a781fe2 $ git log 70b90929b..a781fe29b --date=short --no-merges --format='%ad %ae %s' 2020-07-13 ofats Googletest export 2020-07-11 ashikpaul17 Fixed some minor typos 2020-06-19 amatanhead Make EXPECT_THROW and EXPECT_NO_THROW macros more informative Created with: roll-dep third_party/googletest Roll third_party/re2/ fe8a81adc..ca11026a0 (1 commit) google/re2@fe8a81a...ca11026 $ git log fe8a81adc..ca11026a0 --date=short --no-merges --format='%ad %ae %s' 2020-07-14 junyer Make Regexp::Simplify() return a null pointer when stopped early. Created with: roll-dep third_party/re2 Roll third_party/spirv-headers/ 308bd07..7f2ae11 (1 commit) KhronosGroup/SPIRV-Headers@308bd07...7f2ae11 $ git log 308bd07..7f2ae11 --date=short --no-merges --format='%ad %ae %s' 2020-07-19 vkushwaha Add changes for SPV_EXT_shader_atomic_float Created with: roll-dep third_party/spirv-headers Roll third_party/spirv-tools/ c9b254d..c10d6ce (19 commits) KhronosGroup/SPIRV-Tools@c9b254d...c10d6ce $ git log c9b254d..c10d6ce --date=short --no-merges --format='%ad %ae %s' 2020-07-20 stefanomil spirv-fuzz: refactor to use RemoveAtRandomIndex (KhronosGroup#3560) 2020-07-20 antonikarp spirv-fuzz: add TransformationAddRelaxedDecoration (KhronosGroup#3545) 2020-07-17 alanbaker Store location values sparsely (KhronosGroup#3488) 2020-07-17 dneto Permit Simple and GLSL450 memory model in WEBGPU_0 (KhronosGroup#3463) 2020-07-17 antonikarp spirv-fuzz: support floating-point in TransformationInvertComparisonOperator (KhronosGroup#3551) 2020-07-17 stefanomil Change MaybeApplyTransformation to return a boolean (KhronosGroup#3555) 2020-07-17 stefanomil spirv-fuzz: Implement MaybeApplyTransformation helper function (KhronosGroup#3540) 2020-07-17 stefanomil spirv-fuzz: Assert false in IsApplicable method of TransformationAccessChain (KhronosGroup#3528) 2020-07-16 vasniktel spirv-fuzz: Add support for OpBitcast to TransformationEquationInstruction (KhronosGroup#3523) 2020-07-16 vasniktel spirv-fuzz: Add support for OpConvert to TransformationEquationInstruction (KhronosGroup#3472) 2020-07-15 alanbaker Fix reachability in the validator (KhronosGroup#3541) 2020-07-15 vasniktel spirv-fuzz: Remove TransformationCopyObject (KhronosGroup#3531) 2020-07-15 vasniktel spirv-opt: Add support for OpLabel to dominator analysis (KhronosGroup#3516) 2020-07-15 stefanomil spirv-fuzz: Fuzzer pass to interchange zero-like constants (KhronosGroup#3524) 2020-07-15 afdx spirv-fuzz: Add replay range option (KhronosGroup#3535) 2020-07-14 jaebaek Rewrite KillDebugDeclares() (KhronosGroup#3513) 2020-07-14 andreperezmaselco.developer spirv-fuzz: Fix instruction insertion issue (KhronosGroup#3521) 2020-07-14 andreperezmaselco.developer spirv-fuzz: Implement the OpMatrixTimesMatrix linear algebra case (KhronosGroup#3527) 2020-07-14 greg Add support to GPU-AV instrumentation for Task and Mesh shaders (KhronosGroup#3512) Created with: roll-dep third_party/spirv-tools

Vasniktel mentioned this pull request Jun 28, 2020

spirv-fuzz: TransformationAddSynonyms #3447

Merged

afd suggested changes Jun 30, 2020

View reviewed changes

Vasniktel force-pushed the convert_to_float branch from af89ca8 to 0c0377c Compare July 1, 2020 08:47

Vasniktel marked this pull request as draft July 1, 2020 08:47

Vasniktel force-pushed the convert_to_float branch from 0c0377c to 4370e9d Compare July 2, 2020 13:19

Vasniktel requested a review from afd July 2, 2020 13:20

Vasniktel marked this pull request as ready for review July 2, 2020 13:20

afd suggested changes Jul 3, 2020

View reviewed changes

Vasniktel force-pushed the convert_to_float branch from 4370e9d to a6ad6b3 Compare July 7, 2020 12:10

Vasniktel requested a review from afd July 7, 2020 12:12

afd suggested changes Jul 7, 2020

View reviewed changes

Vasniktel requested a review from afd July 7, 2020 20:19

afd suggested changes Jul 7, 2020

View reviewed changes

source/fuzz/fact_manager.cpp Outdated Show resolved Hide resolved

Vasniktel requested a review from afd July 8, 2020 07:53

Vasniktel added 8 commits July 10, 2020 16:43

Add convertion equations

c719904

Improve tests

5516fd1

fuzzerutil::FindOrCreate* is a separate PR

98c4039

Only one fresh id is required

fef6fbd

Adjust signedness

ba27edf

Minor fixes

6f3e6b4

Update comments

4797f77

Refactor equation facts

5c23143

Vasniktel force-pushed the convert_to_float branch from d6ecf7e to 5c23143 Compare July 11, 2020 08:56

Vasniktel marked this pull request as draft July 12, 2020 09:00

Vasniktel added 3 commits July 12, 2020 13:18

Take equivalence classes into account

54d114a

Remove OpConvertFTo* opcodes

4451127

Improve tests

bfa332e

Vasniktel marked this pull request as ready for review July 12, 2020 11:08

Minor fixes

3df9b21

afd approved these changes Jul 14, 2020

View reviewed changes

afd added the kokoro:run label Jul 14, 2020

kokoro-team removed the kokoro:run label Jul 14, 2020

afd suggested changes Jul 14, 2020

View reviewed changes

source/fuzz/fact_manager.cpp Outdated Show resolved Hide resolved

source/fuzz/fact_manager.cpp Outdated Show resolved Hide resolved

Simplify code

d7d0f49

Vasniktel requested a review from afd July 15, 2020 11:35

paulthomson added the kokoro:run label Jul 15, 2020

kokoro-team removed the kokoro:run label Jul 15, 2020

afd approved these changes Jul 15, 2020

View reviewed changes

afd merged commit b0206b0 into KhronosGroup:master Jul 16, 2020

Vasniktel deleted the convert_to_float branch July 17, 2020 06:51

dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this pull request Aug 19, 2020

spirv-fuzz: Add support for OpConvert to TransformationEquationInstru…

9c83519

…ction (KhronosGroup#3472) Part of KhronosGroup#3440.

Conversation

Vasniktel commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

Vasniktel commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vasniktel commented Jul 1, 2020

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vasniktel commented Jul 12, 2020

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

afd commented Jul 14, 2020

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

afd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Vasniktel commented Jun 28, 2020 •

edited

Loading

Vasniktel commented Jun 30, 2020 •

edited

Loading