Skip to content

Preserve debug info in vector DCE pass#3497

Merged
jaebaek merged 6 commits intoKhronosGroup:masterfrom
jaebaek:dbg_vdce
Jul 10, 2020
Merged

Preserve debug info in vector DCE pass#3497
jaebaek merged 6 commits intoKhronosGroup:masterfrom
jaebaek:dbg_vdce

Conversation

@jaebaek
Copy link
Copy Markdown
Contributor

@jaebaek jaebaek commented Jul 7, 2020

This commit lets the vector DCE pass preserve the OpenCL.DebugInfo.100
information properly. When the vector DCE pass determines the liveness
of instructions, the debug instructions must not affect the decision. In
addition, when it kills some instructions, it has to kill DebugValue
instructions that use the killed instructions. When it updates some
composite values to meaningful values (not undef), it has to remove
DebugValue because the value information becomes incorrect.

@jaebaek jaebaek requested a review from dnovillo July 7, 2020 18:43
@jaebaek jaebaek self-assigned this Jul 7, 2020
@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 7, 2020

@greg-lunarg I cannot add you as a code reviewer, but please give me your feedback if you find something weird for this PR. Thanks!

[&work_list, this, live_components](Instruction* current_inst) {
if (current_inst->GetOpenCL100DebugOpcode() !=
OpenCLDebugInfo100InstructionsMax) {
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar comment to #3492. Add a predicate member to class Instruction. Maybe Instruction::IsDebug()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let me update this code after submitting #3492.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

@dnovillo dnovillo left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. One more use of the new predicate.

void VectorDCE::MarkDebugValueUsesAsDead(Instruction* composite) {
context()->get_def_use_mgr()->ForEachUser(
composite, [this](Instruction* use) {
if (use->GetOpenCL100DebugOpcode() == OpenCLDebugInfo100DebugValue)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change to if (use->IsOpenCL100DebugInstr())?

Copy link
Copy Markdown
Contributor Author

@jaebaek jaebaek Jul 9, 2020

Choose a reason for hiding this comment

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

No. I want to check if this is exactly DebugValue not other debug instructions.

Since I guess that the only debug instruction appears here must be DebugValue, it is not impossible to change it to if (use->IsOpenCL100DebugInstr()) but if (use->GetOpenCL100DebugOpcode() == OpenCLDebugInfo100DebugValue) looks better for both readability and correctness (that I cannot imagine the exact exception case for now).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, yes.

@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 9, 2020

I replaced the class member variable std::vector<Instruction*> dead_dbg_value_ to std::vector<Instruction*> dead_dbg_value.
I followed how it uses live_components and work_list, which are not class member variables but just local variables.

Copy link
Copy Markdown
Contributor

@dnovillo dnovillo left a comment

Choose a reason for hiding this comment

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

I think I'm not understanding why we need to track dead debug value separately here. Why can't they just be treated like any other instructions?

@jaebaek
Copy link
Copy Markdown
Contributor Author

jaebaek commented Jul 9, 2020

We iterate instructions in function->ForEachInst( ... ) or ForEachUse.
If we directly remove an instruction in the middle of the instruction iteration loop, it will have a dangling pointer.
The only purpose of it is to avoid dangling pointers.
Let me add a comment for it.

jaebaek added 6 commits July 9, 2020 21:24
This commit lets the vector DCE pass preserve the OpenCL.DebugInfo.100
information properly. When the vector DCE pass determines the liveness
of instructions, the debug instructions must not affect the decision. In
addition, when it kills some instructions, it has to kill DebugValue
instructions that use the killed instructions. When it updates some
composite values to meaningful values (not undef), it has to remove
DebugValue because the value information becomes incorrect.
@jaebaek jaebaek merged commit a687057 into KhronosGroup:master Jul 10, 2020
@jaebaek jaebaek deleted the dbg_vdce branch July 10, 2020 14:19
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this pull request Aug 19, 2020
This commit lets the vector DCE pass preserve the OpenCL.DebugInfo.100
information properly. When the vector DCE pass determines the liveness
of instructions, the debug instructions must not affect the decision. In
addition, when it kills some instructions, it has to kill DebugValue
instructions that use the killed instructions. When it updates some
composite values to meaningful values (not undef), it has to remove
DebugValue because the value information becomes incorrect.
dneto0 pushed a commit to dneto0/SPIRV-Tools that referenced this pull request Sep 14, 2024
Roll third_party/effcee/ 5af957bbf..2ec8f8738 (3 commits)

google/effcee@5af957b...2ec8f87

$ git log 5af957bbf..2ec8f8738 --date=short --no-merges --format='%ad %ae %s'
2020-06-16 dneto Start v2020.0-dev
2020-06-16 dneto Finalize v2019.1
2020-06-15 dneto Update CHANGES

Created with:
  roll-dep third_party/effcee

Roll third_party/glslang/ e8c9fd6..b481744 (19 commits)

KhronosGroup/glslang@e8c9fd6...b481744

$ git log e8c9fd6..b481744 --date=short --no-merges --format='%ad %ae %s'
2020-07-14 bclayton Give build_info.py the executable bit
2020-07-14 cepheus Fix recently found non-determinism with gl_WorldToObject3x4EXT.
2020-07-14 cepheus Non-determinism: Remove test file that seems to trigger non-determinism.
2020-07-13 bclayton Add bison license to LICENSE.txt
2020-07-13 bclayton CMake: Move project() to top of CMakeLists.txt
2020-07-13 bclayton Kokoro: Print test output to stdout
2020-07-13 cepheus Fix comma in licence checker.
2020-07-13 cepheus Revert "Merge pull request KhronosGroup#2330 from ShabbyX/optimize_for_angle"
2020-07-13 cepheus Fix a couple lines that were too long, to retrigger bots.
2020-07-13 cepheus Fix KhronosGroup#2329: don't use invalid initializers.
2020-07-12 bclayton Add missing comma from license-checker.cfg
2020-07-12 ccom Common: include standard headers before doing any defines
2020-07-10 bclayton Fix CMake rules when nesting CMake projects
2020-07-10 bclayton Attempt to fix chromium builds
2020-06-17 bclayton Generate build information from CHANGES.md
2020-07-03 ShabbyX Customize glslang.y to GLSLANG_ANGLE
2020-07-03 ShabbyX Use GLSLANG_ANGLE to strip features to what ANGLE requires
2020-07-07 rharrison Make sure glslang_angle has a definition in BUILD.gn
2020-07-07 bclayton Use CMake's builtin functionality for PCHs

Created with:
  roll-dep third_party/glslang

Roll third_party/googletest/ 356f2d264..70b90929b (7 commits)

google/googletest@356f2d2...70b9092

$ git log 356f2d264..70b90929b --date=short --no-merges --format='%ad %ae %s'
2020-07-09 absl-team Googletest export
2020-07-07 ofats Googletest export
2020-07-07 absl-team Googletest export
2020-07-07 absl-team Googletest export
2020-04-11 olivier.ldff use target_compile_features to use c++11 if cmake > 3.8
2020-05-30 eli fix compilation on OpenBSD 6.7
2020-01-22 mjvk Fixes extensions missing for QNX

Created with:
  roll-dep third_party/googletest

Roll third_party/spirv-cross/ 559b21c6c..6575e451f (2 commits)

KhronosGroup/SPIRV-Cross@559b21c...6575e45

$ git log 559b21c6c..6575e451f --date=short --no-merges --format='%ad %ae %s'
2020-07-11 post MSVC 2013: Fix silently broken builds.
2020-07-07 troughton MSL: Ensure OpStore source operands are marked for inclusion in function arguments

Created with:
  roll-dep third_party/spirv-cross

Roll third_party/spirv-tools/ 6a4da9d..c9b254d (17 commits)

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

$ git log 6a4da9d..c9b254d --date=short --no-merges --format='%ad %ae %s'
2020-07-14 andreperezmaselco.developer spirv-fuzz: Support adding dead break from back-edge block (KhronosGroup#3519)
2020-07-14 andreperezmaselco.developer Support OpPhi when replacing boolean constant operand (KhronosGroup#3518)
2020-07-12 vasniktel spirv-fuzz: TransformationAddSynonyms (KhronosGroup#3447)
2020-07-11 vasniktel spirv-fuzz: Remove unused functions (KhronosGroup#3510)
2020-07-11 vasniktel spirv-fuzz: Minor refactoring (KhronosGroup#3507)
2020-07-10 greg Preserve OpenCL.DebugInfo.100 through elim-local-single-store (KhronosGroup#3498)
2020-07-10 jaebaek Preserve debug info in vector DCE pass (KhronosGroup#3497)
2020-07-10 stefano.milizia00 Implement transformation to record synonymous constants. (KhronosGroup#3494)
2020-07-09 jaebaek Fix build failure (KhronosGroup#3508)
2020-07-09 greg Upgrade elim-local-single-block for OpenCL.DebugInfo.100 (KhronosGroup#3451)
2020-07-09 vasniktel spirv-fuzz: TransformationReplaceParameterWithGlobal (KhronosGroup#3434)
2020-07-09 andreperezmaselco.developer Implement the OpMatrixTimesVector linear algebra case (KhronosGroup#3500)
2020-07-08 jaebaek Preserve OpenCL.100.DebugInfo in reduce-load-size pass (KhronosGroup#3492)
2020-07-08 andreperezmaselco.developer spirv-fuzz: Add image sample unused components transformation (KhronosGroup#3439)
2020-07-07 andreperezmaselco.developer spirv-fuzz: Add variables with workgroup storage class (KhronosGroup#3485)
2020-07-07 andreperezmaselco.developer spirv-fuzz: Implement the OpVectorTimesMatrix linear algebra case (KhronosGroup#3489)
2020-07-07 vasniktel spirv-fuzz: fuzzerutil::MaybeGetConstant* KhronosGroup#3487

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants