Skip to content

spirv-fuzz: Relax type checking for int contants#3573

Merged
afd merged 8 commits intoKhronosGroup:masterfrom
stefanomil:relax-int-type-checking-transf-record-syn-const
Jul 27, 2020
Merged

spirv-fuzz: Relax type checking for int contants#3573
afd merged 8 commits intoKhronosGroup:masterfrom
stefanomil:relax-int-type-checking-transf-record-syn-const

Conversation

@stefanomil
Copy link
Copy Markdown
Collaborator

@stefanomil stefanomil commented Jul 22, 2020

Right now, TransformationRecordSynonymousConstants requires the type
ids of two candidate constants to be exactly the same.
This PR adds an exception for integer constants, which can be
considered equivalent even if their signedness is different.
This applies to both integers and vector constants.

The IsApplicable method of ReplaceIdWithSynonym is also updated so
that, in the case of two integer constants which don't have the same
type, they can only be swapped in particular instructions (those
that don't take the signedness into consideration).

Fixes #3536 .

@stefanomil stefanomil marked this pull request as ready for review July 22, 2020 15:00
Copy link
Copy Markdown
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.

@stefanomil stefanomil force-pushed the relax-int-type-checking-transf-record-syn-const branch from 9a475cc to 37e7524 Compare July 23, 2020 15:31
@stefanomil stefanomil force-pushed the relax-int-type-checking-transf-record-syn-const branch from 37e7524 to edd4ba3 Compare July 24, 2020 07:47
Copy link
Copy Markdown
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.

A typo - and perhaps some merging problems: there appears to be some needless reformatting, and an introduction of a default parameter.

Copy link
Copy Markdown
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 change introduces some default parameters and some unnecessary changes in formatting. Can you please push again and check that when you diff between your latest changes and the last commit before your change the changes that you see are only those that are truly relevant to the PR?

Copy link
Copy Markdown
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 merged commit 767518e into KhronosGroup:master Jul 27, 2020
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this pull request Aug 19, 2020
Right now, TransformationRecordSynonymousConstants requires the type
ids of two candidate constants to be exactly the same.
This PR adds an exception for integer constants, which can be
considered equivalent even if their signedness is different.
This applies to both integers and vector constants.

The IsApplicable method of ReplaceIdWithSynonym is also updated so
that, in the case of two integer constants which don't have the same
type, they can only be swapped in particular instructions (those
that don't take the signedness into consideration).

Fixes KhronosGroup#3536.
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
* Rolling 2 dependencies and updating expectations

Roll third_party/spirv-cross/ 6575e451f..0376576d2 (5 commits)

KhronosGroup/SPIRV-Cross@6575e45...0376576

$ git log 6575e451f..0376576d2 --date=short --no-merges --format='%ad %ae %s'
2020-07-22 tommek Enabling setting a fixed sampleMask in Metal fragment shaders.
2020-02-20 cdavis MSL: Add support for processing more than one patch per workgroup.
2020-07-22 dsinclair Roll GLSLang, SPIRV-Headers and SPIRV-Tools.
2020-07-22 cdavis MSL: Factor creating a uint type into its own method.
2020-07-22 cdavis MSL: Factor a really gnarly condition into its own method.

Created with:
  roll-dep third_party/spirv-cross

Roll third_party/spirv-tools/ 969f028..b63f0e5 (13 commits)

KhronosGroup/SPIRV-Tools@969f028...b63f0e5

$ git log 969f028..b63f0e5 --date=short --no-merges --format='%ad %ae %s'
2020-07-27 rdb Fix SyntaxWarning in Python 3.8 (KhronosGroup#3388)
2020-07-27 bclayton CMake: Enable building with BUILD_SHARED_LIBS=1 (KhronosGroup#3490)
2020-07-27 dneto Avoid operand type range checks (KhronosGroup#3379)
2020-07-27 jaebaek Preserve debug info in scalar replacement pass (KhronosGroup#3461)
2020-07-27 pierremoreau Update OpenCL capabilities validation (KhronosGroup#3149)
2020-07-27 stevenperron build(deps): bump lodash from 4.17.15 to 4.17.19 in /tools/sva (KhronosGroup#3596)
2020-07-27 antonikarp spirv-fuzz: adds TransformationReplaceLoadStoreWithCopyMemory (KhronosGroup#3586)
2020-07-27 jaebaek Preserve OpenCL.DebugInfo.100 through private-to-local pass (KhronosGroup#3571)
2020-07-27 stefanomil spirv-fuzz: Relax type checking for int contants (KhronosGroup#3573)
2020-07-27 stefanomil spirv-fuzz: Generalise transformation access chain (KhronosGroup#3546)
2020-07-27 stefanomil spirv-fuzz: Split blocks starting with OpPhi before trying to outline (KhronosGroup#3581)
2020-07-27 afdx spirv-fuzz: Set message consumer in replayer when shrinking (KhronosGroup#3591)
2020-07-24 vasniktel spirv-fuzz: Don't use default parameters (KhronosGroup#3583)

Created with:
  roll-dep third_party/spirv-tools

* Add in missing expectations update
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.

spirv-fuzz: Relax type checking in TransformationRecordSynonymousConstants when dealing with integers

3 participants