Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ADCE pass bug for mulitple entries #3470

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Fix ADCE pass bug for mulitple entries #3470

merged 1 commit into from
Jun 29, 2020

Conversation

jaebaek
Copy link
Contributor

@jaebaek jaebaek commented Jun 26, 2020 •
edited
Loading

When there are multiple entries and the shader has a variable with
WorkGroup storage class, those multiple entry functions store values to
the variable. Since ADCE pass uses def-use chains to propagate the work
list, some of instructions in the work list are not actually a part of
the currently processed function. As a result, it adds instructions in
other functions and put them in live_insts_. However, it does not
have the control flow information for those instructions in other
functions i.e., block2headerBranch_ and header2nextHeaderBranch_.
It skips handling those instructions because they are already in
live_insts_ and does not check their control flow information e.g.,
block2headerBranch_ and header2nextHeaderBranch_, which results
in skipping some branches. Even though those branches are live
branches, it considers they are dead branches.

It happens because multiple functions can store values to a variable with
WorkGroup storage class and the code handling store instructions add
them into live_insts_ without considering they are in a function currently
processed. This commit lets it add an instruction to live_insts_ only
when the function is currently processed one.

@jaebaek
Fix ADCE pass bug for mulitple entries
c28af0e 
When there are multiple entries and the shader has a variable with
WorkGroup storage class, those multiple entry functions store values to
the variable. Since ADCE pass uses def-use chains to propagate the work
list, some of instructions in the work list are not actually a part of
the currently processed function. As a result, it adds instructions in
other functions and put them in |live_insts_|. However, it does not
have the control flow information for those instructions in other
functions i.e., |block2headerBranch_| and |header2nextHeaderBranch_|.
When it processes those instructions (they are added when it processes a
different function), it skips handling them because they are already in
|live_insts_| and does not check |block2headerBranch_| and
|header2nextHeaderBranch_|, which results in skipping some branches.
Even though those branches are live branches, it considers they are dead
branches.
@jaebaek
Copy link
Contributor Author

jaebaek commented Jun 26, 2020

This issue is initially reported in DXC github repo microsoft/DirectXShaderCompiler#2828

@jaebaek jaebaek mentioned this pull request Jun 26, 2020
Closed
@jaebaek jaebaek self-assigned this Jun 26, 2020
s-perron
s-perron requested changes Jun 29, 2020
View reviewed changes
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need more time to look at this. I did a quick look before the weekend, and I think this might be missing something if we have not inlined everything. I need to think about it more.

@jaebaek
Copy link
Contributor Author

jaebaek commented Jun 29, 2020

Yeah. Please doublecheck it. I am not 100% sure I understand all the relations between global variables.

s-perron
s-perron approved these changes Jun 29, 2020
View reviewed changes
Copy link
Collaborator

@s-perron s-perron 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. The private_like_local_ flags handles the case I was worried about.

@s-perron s-perron merged commit fc0dc3a into KhronosGroup:master Jun 29, 2020
@jaebaek jaebaek deleted the adce_multiple_entry branch June 29, 2020 19:29
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
@zoddicus
Roll 6 dependencies and update expectations. (KhronosGroup#1097)
1402ed5 
Roll third_party/glslang/ 8397044..f5ed7a6 (30 commits)

KhronosGroup/glslang@8397044...f5ed7a6

$ git log 8397044..f5ed7a6 --date=short --no-merges --format='%ad %ae %s'
2020-07-03 marcin.slusarz Add --quiet option.
2020-07-05 ShabbyX gn: Fix dawn tests in Chromium
2020-07-05 ShabbyX gn: Fix `gn gen --check` by adding missing dependency
2020-07-03 bclayton Add GLSLANG_BUILD_PIC CMake flag
2020-07-03 ShabbyX gn: Optionally disable optimizations and HLSL
2020-07-03 bclayton Don't use add_link_options() on old CMake versions
2020-07-03 bclayton License headers: s/Google/The Khronos Group
2020-07-03 bclayton Kokoro: Correct the `build_file' path to build.sh
2020-07-02 bclayton Add config for license-checker and Kokoro scripts.
2020-07-02 bclayton Fix GLSLANG_IS_SHARED_LIBRARY define
2020-07-01 bclayton Add missing copyright headers
2020-07-02 cepheus Bump revision.
2020-07-01 cepheus SPIRV-Tools and tests: Update to location-validation in SPIRV-Tools.
2020-07-01 cepheus Tests: More broadly use automapping binding/location.
2020-07-01 bclayton Add additional licenses in use to LICENSE.txt
2020-07-01 cepheus HLSL: Catch error cases earlier, preventing a later assert.
2020-06-29 bclayton glslang: Only export public interface for SOs
2020-06-29 bclayton CMake: break up glslang into smaller static libs
2020-06-30 cepheus SPV: RelaxedPrecision: use the result precision for texture sampling.
2020-06-30 cepheus SPV: RelaxedPrecision: Generalize fix KhronosGroup#2293 to cover more operations.
2020-06-24 e.proydakov Fixed GCC -Wunused-parameter in hlslParseables.cpp.
2020-06-29 bclayton CMake: Compile with -fPIC when building SOs
2020-06-29 bclayton CMake: Error on unresolved symbols
2020-06-29 bclayton Remove root kokoro/linux-*-cmake configs
2020-06-26 cepheus SPV: Fix KhronosGroup#2293: keep relaxed precision on arg passed to relaxed param
2020-06-26 cepheus SPV: Partially address KhronosGroup#2293: correct "const in" precision matching.
2020-06-25 lriki.net Add pack_matrix test
2020-06-12 lriki.net HLSL: Fix #pragma pack_matrix(row_major) not work on global uniforms
2020-06-24 bclayton Kokoro: Split linux cmake cfgs into static/shared
2020-06-23 e.proydakov Fixed msvc 2019 nmake compiler warnings with RTTI. By default cmake generates cxx_flags with `/GR` parameter. I updated CMAKE_CXX_FLAGS string and replaced `/GR` -> `/GR-`

Created with:
  roll-dep third_party/glslang

Roll third_party/googletest/ c6e309b26..356f2d264 (10 commits)

google/googletest@c6e309b...356f2d2

$ git log c6e309b26..356f2d264 --date=short --no-merges --format='%ad %ae %s'
2020-07-01 absl-team Googletest export
2020-06-26 absl-team Googletest export
2020-06-25 absl-team Googletest export
2020-06-24 absl-team Googletest export
2020-06-24 absl-team Googletest export
2020-06-19 mayur.shingote Updated googletest issue tracker url.
2020-06-10 rharrison Fix build issue for MinGW
2020-02-21 nini16041988-gitbucket Add missing call for gtest_list_output_unittest_ unitTest. Add unitTest for fixed TEST_P line number. Use CodeLocation TestInfo struct.
2020-02-18 nini16041988-gitbucket Fix: shadow member
2020-02-18 nini16041988-gitbucket Add correct line number to TEST_P test cases for gtest_output.

Created with:
  roll-dep third_party/googletest

Roll third_party/re2/ 14d319322..fe8a81adc (3 commits)

google/re2@14d3193...fe8a81a

$ git log 14d319322..fe8a81adc --date=short --no-merges --format='%ad %ae %s'
2020-07-05 junyer Bump SONAME, which missing ')' versus unexpected ')' needed.
2020-07-03 courbet Make the compiler inline the hot RE2::DFA loop.
2020-06-25 Shikugawa change bazel cpu symbol from wasm to wasm32

Created with:
  roll-dep third_party/re2

Roll third_party/spirv-cross/ f9ae06512..559b21c6c (14 commits)

KhronosGroup/SPIRV-Cross@f9ae065...559b21c

$ git log f9ae06512..559b21c6c --date=short --no-merges --format='%ad %ae %s'
2020-07-06 dsinclair Roll deps.
2020-07-01 post MSL: Do not emit swizzled writes in packing fixups.
2020-07-01 post MSL: Workaround broken vector -> scalar access chain in MSL.
2020-07-06 post MSL: Use input attachment index directly for resource index fallback.
2020-07-06 post GLSL: Support I/O flattening with arrays as final type.
2020-07-03 post GLSL: Support multi-level struct flattening for I/O.
2020-07-01 post Run format_all.sh.
2020-07-01 post test: Use --hlsl-dx9-compatible when attempting to compile SM 3.0 shaders.
2020-06-30 post GLSL: Fix nested legacy switch workarounds.
2020-06-29 post GLSL: Implement switch on ESSL 1.0.
2020-06-29 post GLSL: Use for-loop fallback instead of do/while for legacy ESSL.
2020-06-29 post Implement context-sensitive expression read tracking.
2020-06-29 post Fix bug with control dependent expression tracking.
2020-06-23 post HLSL: Workaround FXC bugs with degenerate switch blocks.

Created with:
  roll-dep third_party/spirv-cross

Roll third_party/spirv-headers/ 11d7637..308bd07 (1 commit)

KhronosGroup/SPIRV-Headers@11d7637...308bd07

$ git log 11d7637..308bd07 --date=short --no-merges --format='%ad %ae %s'
2020-06-26 dj2 Register the Tint compiler

Created with:
  roll-dep third_party/spirv-headers

Roll third_party/spirv-tools/ d4b9f57..6a4da9d (16 commits)

KhronosGroup/SPIRV-Tools@d4b9f57...6a4da9d

$ git log d4b9f57..6a4da9d --date=short --no-merges --format='%ad %ae %s'
2020-07-06 jaebaek Debug info preservation in copy-prop-array pass (KhronosGroup#3444)
2020-07-03 vasniktel spirv-fuzz: TransformationInvertComparisonOperator (KhronosGroup#3475)
2020-07-02 vasniktel Fix regression (KhronosGroup#3481)
2020-07-02 vasniktel spirv-fuzz: Add fuzzerutil::FindOrCreate* (KhronosGroup#3479)
2020-06-30 vasniktel spirv-fuzz: Add FuzzerPassAddCopyMemoryInstructions (KhronosGroup#3391)
2020-06-30 vasniktel spirv-fuzz: Add one parameter at a time (KhronosGroup#3469)
2020-06-29 jaebaek Fix ADCE pass bug for mulitple entries (KhronosGroup#3470)
2020-06-26 ehsannas Add gl_BaseInstance to the name mapper. (KhronosGroup#3462)
2020-06-26 andreperezmaselco.developer Implement the OpMatrixTimesScalar linear algebra case (KhronosGroup#3450)
2020-06-25 jaebaek Clear debug information for kill and replacement (KhronosGroup#3459)
2020-06-25 alanbaker Validate location assignments (KhronosGroup#3308)
2020-06-23 ehsannas Support OpCompositeExtract pattern in desc_sroa (KhronosGroup#3456)
2020-06-23 vasniktel spirv-fuzz: Implement FuzzerPassAddParameters (KhronosGroup#3399)
2020-06-23 vasniktel spirv-fuzz: Add GetParameters (KhronosGroup#3454)
2020-06-23 vasniktel spirv-fuzz: Permute OpPhi instruction operands (KhronosGroup#3421)
2020-06-22 rharrison Add support for different default/trunks in roll-deps (KhronosGroup#3442)

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
Reviewers

@alan-baker alan-baker alan-baker approved these changes

@s-perron s-perron s-perron approved these changes

@dnovillo dnovillo Awaiting requested review from dnovillo

Assignees

@jaebaek jaebaek

Labels
component:optimization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jaebaek @s-perron @alan-baker