Clear debug information for kill and replacement #3459

jaebaek · 2020-06-23T22:09:51Z

For many spirv-opt passes such as simplify-instructions pass, we have to
correctly clear the OpenCL.DebugInfo.100 debug information for
KillInst() and ReplaceAllUses(). If we keep some debug information that
disappeared because of KillInst() and ReplaceAllUses(), adding new
DebugValue instructions will generate incorrect information. This CL
update DebugInfoManager and IRContext to correctly clear debug
information.

For many spirv-opt passes such as simplify-instructions pass, we have to correctly clear the OpenCL.DebugInfo.100 debug information for KillInst() and ReplaceAllUses(). If we keep some debug information that disappeared because of KillInst() and ReplaceAllUses(), adding new DebugValue instructions will generate incorrect information. This CL update DebugInfoManager and IRContext to correctly clear debug information.

jaebaek · 2020-06-24T19:25:23Z

@greg-lunarg I think you need this PR for the debug information preservation in ADCE. Could you please review the PR if it would have some issues with ADCE?

s-perron

Looks good. I like this type of testing. It is more foundational. Just a couple minor changes.

source/opt/debug_info_manager.h

s-perron · 2020-06-24T20:09:17Z

source/opt/debug_info_manager.h

-  std::unordered_map<uint32_t, std::vector<Instruction*>> var_id_to_dbg_decl_;
+  // Mapping from variable or value ids to DebugDeclare or DebugValue
+  // instructions whose operand is the variable or value.
+  std::unordered_map<uint32_t, std::list<Instruction*>> var_id_to_dbg_decl_;


Why did you change to a list?

This is a minor issue because I do not think it will have many elements. The list will have many elements only when we have many reference variables.

The line 565 of source/opt/debug_info_manager.cpp removes a single DebugDeclare or DebugValue from the std::list<Instruction*> of var_id_to_dbg_decl_ to clear the DebugDeclare or DebugValue from the data structure of DebugInfoManager.

Since std::vector::erase calls the destructor and the constructor O(N) times (N = number of elem shift), I thought std::list<Instruction*> is more efficient, but I just realized that std::unordered_set would be better.

We use it for inserting a single element at a time (order does not matter), iterating all elements (order does not matter), and deleting a single random element at a time.
I want to change it to std::unordered_set if you do not have a strong concern.

unorder_set is fine. Avoid std::list, it is known to have poor implementations. In general, if you know things will be small, you can never go wrong with std::vector. It has so much less overhead than the other containers, that in practice it can still be faster.

std::vector::erase calls the destructor and the constructor O(N) times

A pointer does not have a constructor or destructor, so it will not be called.

I see. Thank you for the explanation!

source/opt/ir_context.cpp

jaebaek

Thank you for your comments. I updated it. PTAL!

source/opt/debug_info_manager.h

jaebaek · 2020-06-25T02:50:07Z

source/opt/debug_info_manager.h

-  std::unordered_map<uint32_t, std::vector<Instruction*>> var_id_to_dbg_decl_;
+  // Mapping from variable or value ids to DebugDeclare or DebugValue
+  // instructions whose operand is the variable or value.
+  std::unordered_map<uint32_t, std::list<Instruction*>> var_id_to_dbg_decl_;


This is a minor issue because I do not think it will have many elements. The list will have many elements only when we have many reference variables.

The line 565 of source/opt/debug_info_manager.cpp removes a single DebugDeclare or DebugValue from the std::list<Instruction*> of var_id_to_dbg_decl_ to clear the DebugDeclare or DebugValue from the data structure of DebugInfoManager.

Since std::vector::erase calls the destructor and the constructor O(N) times (N = number of elem shift), I thought std::list<Instruction*> is more efficient, but I just realized that std::unordered_set would be better.

We use it for inserting a single element at a time (order does not matter), iterating all elements (order does not matter), and deleting a single random element at a time.
I want to change it to std::unordered_set if you do not have a strong concern.

source/opt/ir_context.cpp

s-perron · 2020-06-25T15:24:52Z

test/opt/local_ssa_elim_test.cpp

-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_f]] %float_0
-; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue [[dbg_x]] %float_0
+; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue {{%\w+}} %float_0
+; CHECK-NEXT: OpExtInst %void [[ext]] DebugValue {{%\w+}} %float_0


Why are you not checking the local variables anymore? If the issue is order, you can use CHECK-DAG.

Thanks. CHECK-DAG is what I needed.

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

jaebaek added the component:optimization label Jun 23, 2020

jaebaek requested a review from s-perron June 23, 2020 22:09

jaebaek self-assigned this Jun 23, 2020

s-perron requested changes Jun 24, 2020

View reviewed changes

Updated based on code review

3c3ad09

jaebaek commented Jun 25, 2020

View reviewed changes

Fix unit test failure

91e546c

s-perron reviewed Jun 25, 2020

View reviewed changes

Restore unit test and use DAG for order issue

ff7e23d

s-perron approved these changes Jun 25, 2020

View reviewed changes

jaebaek merged commit efaae24 into KhronosGroup:master Jun 25, 2020

jaebaek deleted the clear_dbg_info branch June 25, 2020 19:48

jaebaek mentioned this pull request Jul 6, 2020

Debug information preservation in simplification pass #3491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear debug information for kill and replacement #3459

Clear debug information for kill and replacement #3459

jaebaek commented Jun 23, 2020

jaebaek commented Jun 24, 2020 •

edited

Loading

s-perron left a comment

s-perron Jun 24, 2020

jaebaek Jun 25, 2020 •

edited

Loading

s-perron Jun 25, 2020

jaebaek Jun 25, 2020

jaebaek left a comment

jaebaek Jun 25, 2020 •

edited

Loading

s-perron Jun 25, 2020

jaebaek Jun 25, 2020

Clear debug information for kill and replacement #3459

Clear debug information for kill and replacement #3459

Conversation

jaebaek commented Jun 23, 2020

jaebaek commented Jun 24, 2020 • edited Loading

s-perron left a comment

Choose a reason for hiding this comment

s-perron Jun 24, 2020

Choose a reason for hiding this comment

jaebaek Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

s-perron Jun 25, 2020

Choose a reason for hiding this comment

jaebaek Jun 25, 2020

Choose a reason for hiding this comment

jaebaek left a comment

Choose a reason for hiding this comment

jaebaek Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

s-perron Jun 25, 2020

Choose a reason for hiding this comment

jaebaek Jun 25, 2020

Choose a reason for hiding this comment

jaebaek commented Jun 24, 2020 •

edited

Loading

jaebaek Jun 25, 2020 •

edited

Loading

jaebaek Jun 25, 2020 •

edited

Loading