Implement transformation to interchange zero-like constants#3486
Implement transformation to interchange zero-like constants#3486stefanomil wants to merge 18 commits intoKhronosGroup:masterfrom
Conversation
…f constant definition is necessary), write tests for IsApplicable method.
…ass and write corresponding tests.
|
|
afd
left a comment
There was a problem hiding this comment.
I'm really impressed with this PR - it's structurally sound and shows that you have understood a lot about the framework!
I started doing a thorough review and added a few comments - but then I had an idea of a different way we could do this that I think will be a bit more general and will re-use existing code. (I find this often happens during code review.)
I will follow up with a comment to explain my idea.
| // Choose randomly whether to apply the transformation | ||
| if (!GetFuzzerContext()->ChoosePercentage( | ||
| GetFuzzerContext()->GetChanceOfTogglingConstantNull())) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
My overall feedback is that we should do this PR a different way.
However, before I realised that I had written this (which is sound advice in general even if it no longer applies here.)
It's best to do this check earlier - this is a super-cheap check, and there's no point making the transformation and checking whether it is applicable if we may probabilistically decide not to do the transformation.
| // The result id of the constant declaration instruction | ||
| uint32 constant_id = 1; |
There was a problem hiding this comment.
Again, this is sort of obsolete given my overall comment on the PR, but is perhaps worth digesting anyway.
I think it would be better for us to do this on a per-use basis.
For example, if %a is OpConstantFalse of type bool, and %b is OpConstantNull of type bool, and if the module contains many uses of %a then it should be possible to transform just one of those uses to %b.
To do this, use an IdUseDescriptor.
|
In spirv-fuzz there is a "fact manager" that tracks facts about the module being transformed. One kind of fact is called a "data synonym" fact, and it records that two pieces of data are known to be equal. In the simplest case - which suffices for this discussion - it tracks that two ids are equal. There is a transformation called TransformationReplaceIdWithSynonym that takes some use of an id %a, and an id %b that is synonymous with %a, and replaces the use of %a with a use of %b. For example, there might be an instruction: %c = OpIAdd %int %a %a ; This is like c = a + a If the fact manager knows that %a and %b are synonymous (where %b is some other id) then TransformationReplaceIdWithSynonym could say "replace the first use of %a in '%c = OpIAdd %int %a %a' with %b". That would turn the above instruction into: %c = OpIAdd %int %b %a ; This is like c = b + a and this is semantics-preserving if the fact that %a and %b are synonymous really is true. Now, what does this have to do with interchanging zero-like constants? Well: we could write a fuzzer pass that does the following:
** This will need a new transformation, TransformationRecordSynonymousConstants, which should be applicable if two given constants really are equivalent and should have the effect of adding the fact that they are synonymous to the fact manager - it should not actually change the module *** This will require applying a TransformationReplaceIdWithSynonym I suggest you take a look at FuzzerPassApplyIdSynonyms to see some related code. Let me know if this makes sense - keen to discuss. |
…and add skeleton files for its implementation.
…structor and ToMessage method of TransformationRecordSynonymousConstants class.
…nts class and implement first tests for it.
…d integrate tests to check its functionality.
|
I have implemented the fuzzer pass and the transformation as described. For each zero-like constant %c, the fuzzer pass looks for an existing toggled instruction and adds a new one only if it doesn't find it, otherwise it uses the already-existing one (there were some functions in the fuzzer_pass file that I could reuse to do that). I was unsure whether the IsApplicable function of the transformation class should return true with two constants that are equal (same opcode and same operands). I opted for also returning true in this case (even though this is not used by the pass) because in my mind it seemed to make more sense this way. Let me know about any observations or corrections to make. |
afd
left a comment
There was a problem hiding this comment.
This is looking good!
I have just reviewed the content related to TransformationRecordSynonymousConstants.
I think - from a code review perspective - it would best to do a small-ish PR that just introduces this class and its associated tests.
Would you be able to do such a PR, based on my feedback?
Once we get that merged I will find it easier to review the code that uses it.
| TransformationAddTypeInt add_type_int = 7; | ||
| TransformationAddDeadBreak add_dead_break = 8; | ||
| TransformationReplaceBooleanConstantWithConstantBinary | ||
| replace_boolean_constant_with_constant_binary = 9; |
There was a problem hiding this comment.
Avoid unnecessary whitespace changes. I do this by doing a git diff against master before submitting my PR and carefully checking that the only changes are directly relevant to my PR.
| // Two constants are synonymous if one of them is either OpConstantFalse | ||
| // (type bool) or zero-valued OpConstant of type int or float and the other | ||
| // is OpConstantNull of the same type. |
There was a problem hiding this comment.
The language needs to be tighter here and we should try to be reasonably general.
We should regard two constants as being equal - and thus appropriate for use with this fact - if:
- they have the same type, ignoring integer sign
- they have the same opcode, which can be one of OpConstant, OpConstantTrue, OpConstantFalse, OpConstantNull
- They have the same value (in the case of OpConstant and OpConstantTrue/True)
As a special case, as long as types match, OpConstantNull is equivalent to OpConstantFalse, to OpConstant with all data words set to zero.
(In the future we should also handle OpConstantComposite, but let's not do that now - let's open an issue for it.)
| // The id of a constant | ||
| uint32 constant_id = 1; | ||
|
|
||
| // The id of the synonym | ||
| uint32 synonym_id = 2; |
There was a problem hiding this comment.
I would call them "constant1_id" and "constant2_id"
| if (!constant->type()->IsSame(synonym->type())) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
A refinement to this: if the types are both integer, with the same width, then we don't care if their signedness is different. In that case we'd want to check that their associated data words are the same.
| namespace fuzz { | ||
|
|
||
| namespace { | ||
| static inline bool IsStaticZeroConstant( |
There was a problem hiding this comment.
Remove static - it's redundant since you're in an anonymous namespace.
Remove inline - the code isn't performance critical.
Change name to IsScalarZeroConstant.
|
I have opened a new pull request (#3494 ) that just introduces the transformation class and tests, so I am closing this one. |
This fuzzer pass: For each zero-like constant, either finds the existing definition of the corresponding toggled one (OpConstantNull becomes zero-valued scalar OpConstant or vice versa) or creates a new one if it doesn't exist and records that the two are synonyms For each use of these constants, probabilistically decides whether to change it with the corresponding toggled constant id (as described in #3486 ) Only uses inside blocks of instructions are considered and not, for example, in instructions declaring other constants.
…oup#3524) This fuzzer pass: For each zero-like constant, either finds the existing definition of the corresponding toggled one (OpConstantNull becomes zero-valued scalar OpConstant or vice versa) or creates a new one if it doesn't exist and records that the two are synonyms For each use of these constants, probabilistically decides whether to change it with the corresponding toggled constant id (as described in KhronosGroup#3486 ) Only uses inside blocks of instructions are considered and not, for example, in instructions declaring other constants.
…oup#3524) This fuzzer pass: For each zero-like constant, either finds the existing definition of the corresponding toggled one (OpConstantNull becomes zero-valued scalar OpConstant or vice versa) or creates a new one if it doesn't exist and records that the two are synonyms For each use of these constants, probabilistically decides whether to change it with the corresponding toggled constant id (as described in KhronosGroup#3486 ) Only uses inside blocks of instructions are considered and not, for example, in instructions declaring other constants.
#3132
I have implemented the TransformationToggleConstantNull transformation, tests and fuzzer pass, which transforms zero-valued constants of type int, float or bool into null constants of the same type (according to the specification, a null constant of these types is equivalent to a constant with value zero - false for booleans).
Is the code generally ok? Are there other cases that I should have considered?
I chose the upper and lower bounds for the probability of applying the transformation to be 20 and 90 a bit randomly.
I am also not sure if the name is explicative enough.