spirv-fuzz: TransformationReplaceAddSubMulWithCarryingExtended#3598
Conversation
afd
left a comment
There was a problem hiding this comment.
A great start! Please see comments.
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.h
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.h
Outdated
Show resolved
Hide resolved
f848727 to
34fe3ee
Compare
paulthomson
left a comment
There was a problem hiding this comment.
This looks really good. A few minor suggestions.
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
test/fuzz/transformation_replace_add_sub_mul_with_carrying_extended_test.cpp
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
34fe3ee to
1176050
Compare
Vasniktel
left a comment
There was a problem hiding this comment.
This looks very nice! You've correctly figured out that the signedness of operands of OpIAdd, OpISub and OpIMul can be different.
I was a bit pedantic, though, and left some refactoring comments. I've also suggested to remove struct_type_id from the protobuf message.
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
| uint32_t operand_1_type_id = | ||
| ir_context->get_def_use_mgr() | ||
| ->GetDef(instruction->GetSingleWordInOperand( | ||
| kArithmeticInstructionIndexLeftInOperand)) | ||
| ->type_id(); | ||
|
|
||
| uint32_t operand_2_type_id = | ||
| ir_context->get_def_use_mgr() | ||
| ->GetDef(instruction->GetSingleWordInOperand( | ||
| kArithmeticInstructionIndexRightInOperand)) | ||
| ->type_id(); |
There was a problem hiding this comment.
Operands can be vectors.
There was a problem hiding this comment.
If operands are vectors then the result is also a vector. And both type_ids of operands and the result type_id must be equal.
There was a problem hiding this comment.
Yes, you are right! Sorry for the confusion.
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
Vasniktel
left a comment
There was a problem hiding this comment.
Looks very nice! It would be great if you could add tests for the cases when operands of OpIAdd etc are vectors.
|
Builds fail because of the unused |
Thanks, I'll fix it. |
paulthomson
left a comment
There was a problem hiding this comment.
This looks really good, and very easy to read thanks to the good comments! I just noticed that I don't think you need some of the switches.
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
paulthomson
left a comment
There was a problem hiding this comment.
This is very close. I was a bit unclear with my last comments! I have clarified here. I have also left a few potentially pedantic suggestions about comments; when the comments don't appear to match the code, I get scared.
And a few important comments about tests.
source/fuzz/fuzzer_pass_replace_adds_subs_muls_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
test/fuzz/transformation_replace_add_sub_mul_with_carrying_extended_test.cpp
Outdated
Show resolved
Hide resolved
test/fuzz/transformation_replace_add_sub_mul_with_carrying_extended_test.cpp
Outdated
Show resolved
Hide resolved
test/fuzz/transformation_replace_add_sub_mul_with_carrying_extended_test.cpp
Show resolved
Hide resolved
test/fuzz/transformation_replace_add_sub_mul_with_carrying_extended_test.cpp
Show resolved
Hide resolved
source/fuzz/transformation_replace_add_sub_mul_with_carrying_extended.cpp
Outdated
Show resolved
Hide resolved
4560510 to
43b96a9
Compare
paulthomson
left a comment
There was a problem hiding this comment.
Awesome! Very nice tests.
…osGroup#3598) Replaces OpIAdd with OpIAddCarry, OpISub with OpISubBorrow, OpIMul with OpUMulExtended or OpSMulExtended and stores the result into a fresh_id representing a structure. Extracts the first element of the result into the original result_id. This value is the same as the result of the original instruction. Fixes KhronosGroup#3577
Roll third_party/glslang/ 3ee5f2f..b60e067 (8 commits) KhronosGroup/glslang@3ee5f2f...b60e067 $ git log 3ee5f2f..b60e067 --date=short --no-merges --format='%ad %ae %s' 2020-08-06 john SPV: Fix KhronosGroup#1829: don't emit OpModuleProcessed use-storage-buffer 2020-08-05 john Build/Test: Dropping 2013 allows using the latest googletests. 2020-08-04 john SPV: Standalone; sanity check the client GLSL input semantics option value. 2020-08-04 john SPV: Use more correct SPV-Tools environment, partially addressing KhronosGroup#2290 2020-08-04 john SPV: Fix KhronosGroup#2363: include trailing newline named text SPV output. 2020-07-03 ShabbyX Use GLSLANG_ANGLE to strip features to what ANGLE requires 2020-07-31 bclayton Revert changes that migrate to `thread_local`. 2020-07-27 dneto Avoid spurious warning about uninit var Created with: roll-dep third_party/glslang Roll third_party/googletest/ a781fe29b..3af06fe16 (12 commits) google/googletest@a781fe2...3af06fe $ git log a781fe29b..3af06fe16 --date=short --no-merges --format='%ad %ae %s' 2020-08-05 absl-team Googletest export 2020-08-03 absl-team Googletest export 2020-08-03 absl-team Googletest export 2020-08-05 zumix.cpp fix endif comment 2020-08-02 zumix.cpp fix tests 2020-07-29 franciscogthiesen Removing tiny-dnn from "Who is using.." 2020-07-28 absl-team Googletest export 2020-07-29 zumix.cpp fix GTEST_REMOVE_LEGACY_TEST_CASEAPI_ typo 2020-07-28 absl-team Googletest export 2020-07-26 ofats Googletest export 2020-07-19 jasjuang fix clang tidy modernize-use-equals-default warnings 2020-07-02 siliconearth Fix test failing when simple regex is used Created with: roll-dep third_party/googletest Roll third_party/spirv-cross/ 0376576d2..82d1c43e4 (7 commits) KhronosGroup/SPIRV-Cross@0376576...82d1c43 $ git log 0376576d2..82d1c43e4 --date=short --no-merges --format='%ad %ae %s' 2020-08-03 cdavis MSL: Fix handling of matrices and structs in the output control point array. 2020-07-29 post Add some test cases for complex type aliasing scenario. 2020-07-29 post Ensure that we use primary alias type when emitting flattened members. 2020-07-29 post GLSL: Be more aggressive about using type_alias. 2020-07-29 post Only rewrite type aliases for the base type. 2020-07-28 post GLSL: Add option to force flattening IO blocks. 2020-07-23 tommek Adding BuiltInSampleMask in HLSL Created with: roll-dep third_party/spirv-cross Roll third_party/spirv-headers/ 979924c..3fdabd0 (4 commits) KhronosGroup/SPIRV-Headers@979924c...3fdabd0 $ git log 979924c..3fdabd0 --date=short --no-merges --format='%ad %ae %s' 2020-08-03 44190824+mmerecki Reserve SPIR-V token range for upcoming Intel extensions. (KhronosGroup#165) 2020-07-29 alanbaker Update BUILD.bazel and BUILD.gn (KhronosGroup#166) 2020-07-29 alanbaker Publish the headers for the clspv embedded reflection non-semantic extended instruction set (KhronosGroup#164) 2020-07-29 johnkslang Update the registry in spir-v.xml to modernize and split out opcodes. (KhronosGroup#156) Created with: roll-dep third_party/spirv-headers Roll third_party/spirv-tools/ b63f0e5..2990a21 (30 commits) KhronosGroup/SPIRV-Tools@b63f0e5...2990a21 $ git log b63f0e5..2990a21 --date=short --no-merges --format='%ad %ae %s' 2020-08-10 stevenperron Avoid using /MP4 for clang on windows. (KhronosGroup#3662) 2020-08-06 antonikarp spirv-fuzz: TransformationReplaceAddSubMulWithCarryingExtended (KhronosGroup#3598) 2020-08-06 andreperezmaselco.developer spirv-fuzz: Add TransformationMakeVectorOperationDynamic (KhronosGroup#3597) 2020-08-06 andreperezmaselco.developer spirv-fuzz: iterate over blocks in replace linear algebra pass (KhronosGroup#3654) 2020-08-06 stefanomil spirv-fuzz: make outliner pass use additional transformations (KhronosGroup#3604) 2020-08-05 jaebaek OpenCL.DebugInfo.100 DebugTypeArray with variable size (KhronosGroup#3549) 2020-08-05 andreperezmaselco.developer spirv-opt: Improve the code of the Instruction class (KhronosGroup#3610) 2020-08-05 vasniktel spirv-fuzz: Handle OpPhis in livesafe functions (KhronosGroup#3642) 2020-08-05 vasniktel spirv-fuzz: Handle OpPhi during constant obfuscation (KhronosGroup#3640) 2020-08-05 vasniktel spirv-fuzz: Fix FuzzerPassCopyObjects (KhronosGroup#3638) 2020-08-04 vasniktel spirv-fuzz: Remove OpFunctionCall operands in correct order (KhronosGroup#3630) 2020-08-04 vasniktel spirv-fuzz: Handle capabilities during module donation (KhronosGroup#3651) 2020-08-04 vasniktel spirv-fuzz: Refactor boilerplate in TransformationAddParameter (KhronosGroup#3625) 2020-08-03 vasniktel spirv-fuzz: TransformationMoveInstructionDown (KhronosGroup#3477) 2020-07-31 jaebaek Remove DebugDeclare only for target variables in ssa-rewrite (KhronosGroup#3511) 2020-07-31 vasniktel Fix typo in ASAN CI build (KhronosGroup#3623) 2020-07-30 stefanomil spirv-fuzz: Transformation to add loop preheader (KhronosGroup#3599) 2020-07-30 stefanomil spirv-fuzz: Pass to replace int operands with ints of opposite signedness (KhronosGroup#3612) 2020-07-30 jaebaek Debug info preservation in loop-unroll pass (KhronosGroup#3548) 2020-07-30 alanbaker Validator support for non-semantic clspv reflection (KhronosGroup#3618) 2020-07-30 vasniktel spirv-fuzz: Fix memory bugs (KhronosGroup#3622) 2020-07-29 andreperezmaselco.developer spirv-fuzz: Implement the OpOuterProduct linear algebra case (KhronosGroup#3617) 2020-07-30 vasniktel spirv-fuzz: Compute corollary facts from OpBitcast (KhronosGroup#3538) 2020-07-29 dj2 Update some language usage. (KhronosGroup#3611) 2020-07-29 vasniktel spirv-fuzz: Relax type constraints in DataSynonym facts (KhronosGroup#3602) 2020-07-29 vasniktel spirv-fuzz: Remove non-deterministic behaviour (KhronosGroup#3608) 2020-07-29 afdx Avoid use of 'sanity' and 'sanity check' in the code base (KhronosGroup#3585) 2020-07-27 andreperezmaselco.developer spirv-fuzz: Add condition to make functions livesafe (KhronosGroup#3587) 2020-07-27 rharrison Rolling 4 dependencies (KhronosGroup#3601) 2020-07-27 andreperezmaselco.developer spirv-fuzz: Implement the OpTranspose linear algebra case (KhronosGroup#3589) Created with: roll-dep third_party/spirv-tools
Replaces OpIAdd with OpIAddCarry, OpISub with OpISubBorrow, OpIMul with
OpUMulExtended or OpSMulExtended and stores the result into a fresh_id
representing a structure. Extracts the first element of the result into
the original result_id. This value is the same as the result of the
original instruction.
Fixes #3577