Skip to content

CCP should mark IR changed if it created new constants.#3732

Merged
dnovillo merged 2 commits intoKhronosGroup:masterfrom
dnovillo:3636
Aug 20, 2020
Merged

CCP should mark IR changed if it created new constants.#3732
dnovillo merged 2 commits intoKhronosGroup:masterfrom
dnovillo:3636

Conversation

@dnovillo
Copy link
Copy Markdown
Contributor

This fixes #3636.

When CCP is simulating statements, it will sometimes successfully fold
an instruction, which laters switches to varying. The initial fold of
the instruction may generate a new constant K.

The problem we were running into is when K never gets propagated to the
IR. Its definition will still exist, so CCP should mark the IR modified
in this case.

In fixing this bug, I noticed that an existing test was suffering from
the same bug. The change also makes PassTest::SinglePassRunAndMatch()
return the result from the pass, so that we can check that the pass
marks the IR modified in this case.

This fixes KhronosGroup#3636.

When CCP is simulating statements, it will sometimes successfully fold
an instruction, which laters switches to varying.  The initial fold of
the instruction may generate a new constant K.

The problem we were running into is when K never gets propagated to the
IR.  Its definition will still exist, so CCP should mark the IR modified
in this case.

In fixing this bug, I noticed that an existing test was suffering from
the same bug.  The change also makes PassTest::SinglePassRunAndMatch()
return the result from the pass, so that we can check that the pass
marks the IR modified in this case.
@dnovillo dnovillo requested a review from s-perron August 20, 2020 19:35
@dnovillo dnovillo self-assigned this Aug 20, 2020
@dnovillo dnovillo merged commit b79773a into KhronosGroup:master Aug 20, 2020
dnovillo added a commit to dnovillo/SPIRV-Tools that referenced this pull request Oct 27, 2020
The previous attempts at fixing this issue relied on marking the IR
changed only when CCP was able to fold an instruction during
propagation (KhronosGroup#3799,
KhronosGroup#3732).

Those fixes missed the case described in
KhronosGroup#3991.  In this case,
the folder never actually succeeds in folding the instruction, but it
does create constants in the process.

Fixed with this change.
dnovillo added a commit that referenced this pull request Oct 28, 2020
Simplify logic to decide whether CCP modified the IR.

The previous attempts at fixing this issue relied on marking the IR
changed only when CCP was able to fold an instruction during
propagation (#3799,
#3732).

Those fixes missed the case described in
#3991.  In this case,
the folder never actually succeeds in folding the instruction, but it
does create constants in the process.

Fixed with this change.
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
Roll third_party/glslang/ f257e0e..983698b (2 commits)

KhronosGroup/glslang@f257e0e...983698b

$ git log f257e0e..983698b --date=short --no-merges --format='%ad %ae %s'
2020-08-23 john Revert "Merge pull request KhronosGroup#2371 from RafaelMarinheiro/master"
2020-08-21 julius.ikkala Obey ENABLE_PCH CMake option

Created with:
  roll-dep third_party/glslang

Roll third_party/googletest/ adeef1929..1e315c5b1 (14 commits)

google/googletest@adeef19...1e315c5

$ git log adeef1929..1e315c5b1 --date=short --no-merges --format='%ad %ae %s'
2020-08-20 absl-team Googletest export
2020-08-17 absl-team Googletest export
2020-08-12 robert.earhart Export LICENSE
2020-08-03 amatanhead Remove ThrowsMessageHasSubstr and fix some nits after review
2020-07-13 amatanhead Cleanup a bulky expression, document implementation details
2020-07-07 amatanhead Fix build under msvc
2020-07-07 amatanhead Update tests after changing an error message
2020-07-07 amatanhead Fix build under msvc
2020-07-07 amatanhead Add a test to ensure that the `Throws` matcher only invokes its argument once.
2020-07-07 amatanhead Add a test for duplicate catch clauses in throw matchers, fix a couple of nitpicks.
2020-07-03 amatanhead Add missing documentation piece
2020-06-20 amatanhead Small improvements: code style and property name
2020-06-20 amatanhead Add matchers for testing exception properties
2018-05-01 lantw44 Avoid using environ on FreeBSD

Created with:
  roll-dep third_party/googletest

Roll third_party/spirv-cross/ 4c7944bb4..685f86471 (6 commits)

KhronosGroup/SPIRV-Cross@4c7944b...685f864

$ git log 4c7944bb4..685f86471 --date=short --no-merges --format='%ad %ae %s'
2020-08-24 post Run format_all.sh.
2020-08-24 post Work around annoying warning on GCC 10.2.
2020-08-21 post Overhaul how we deal with reserved identifiers.
2020-08-20 post HLSL: Fix FragCoord.w.
2020-08-20 post HLSL: Deal with partially filled 16-byte word in cbuffers.
2020-08-20 post HLSL: Fix bug in is_packing_standard for cbuffer.

Created with:
  roll-dep third_party/spirv-cross

Roll third_party/spirv-tools/ b8de4f5..4dd1223 (9 commits)

KhronosGroup/SPIRV-Tools@b8de4f5...4dd1223

$ git log b8de4f5..4dd1223 --date=short --no-merges --format='%ad %ae %s'
2020-08-21 andreperezmaselco.developer spirv-fuzz: Add words instead of logical operands (KhronosGroup#3728)
2020-08-20 dnovillo CCP should mark IR changed if it created new constants. (KhronosGroup#3732)
2020-08-19 antonikarp spirv-fuzz: add FuzzerPassAddCompositeInserts (KhronosGroup#3606)
2020-08-19 antonikarp spirv-fuzz: Support pointer types in FuzzerPassAddParameters (KhronosGroup#3627)
2020-08-18 jaebaek Let ADCE pass check DebugScope (KhronosGroup#3703)
2020-08-18 andreperezmaselco.developer spirv-opt: Implement opt::Function::HasEarlyReturn function (KhronosGroup#3711)
2020-08-17 andreperezmaselco.developer spirv-fuzz: Check termination instructions when donating modules (KhronosGroup#3710)
2020-08-17 jackoalan Fix -Wrange-loop-analysis warning (KhronosGroup#3712)
2020-08-17 andreperezmaselco.developer spirv-fuzz: Check header dominance when adding dead block (KhronosGroup#3694)

Created with:
  roll-dep third_party/spirv-tools
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-opt: CCP pass might create a new constant and return SuccessWithoutChange

2 participants