Preserve OpenCL.DebugInfo.100 through elim-local-single-store by greg-lunarg · Pull Request #3498 · KhronosGroup/SPIRV-Tools

greg-lunarg · 2020-07-07T19:39:14Z

This pass basically follows the same process as ssa-rewrite: it adds a DebugValue after each Store and removes the DebugDeclare or DebugValue Deref. It only does this if all instructions that are dependent on the Store are Loads and are replaced.

greg-lunarg · 2020-07-07T19:50:11Z

@jaebaek @s-perron Please review.

jaebaek

Thank you for working on it!

jaebaek · 2020-07-08T14:43:54Z

source/opt/local_single_store_elim_pass.cpp

+        if (dbg_op == OpenCLDebugInfo100DebugDeclare ||
+            dbg_op == OpenCLDebugInfo100DebugValue) {
+          break;
+        }


Nit:
A debug instruction other than DebugDeclare and DebugValue must not reference the result id of OpVariable.
Please add an assert just before the following return nullptr; like assert(false && "Debug instruction other than DebugDeclare and DebugValue must not reference OpVariable");.

Because we have a validation pass that is run by default at the beginning of spirv-opt, we can and do assume that every SPIR-V instruction is valid. This keeps our code from being cluttered and slowed down with repetitive validity checks. In light of this, could we agree this assert is not necessary? Or if it is necessary, a better place for it is in the validation pass?

Ah I see. Never mind it.

jaebaek · 2020-07-08T19:54:47Z

source/opt/local_single_store_elim_pass.cpp

+    const analysis::Type* var_type =
+        context()->get_type_mgr()->GetType(var_inst->type_id());
+    const analysis::Type* store_type = var_type->AsPointer()->pointee_type();
+    if (!(store_type->AsStruct() || store_type->AsArray())) {


Could you please let me know why we must skip an aggregate?

Based partly on a concern from @s-perron who thought we should not have DebugValue of aggregate (array or struct) type. The result of this logic is that arrays and structs will stay in memory and keep their DebugDeclare. I think this is a completely acceptable result.

Really, with loop unrolling and scalar renaming, we should almost never see aggregate types at this point. If we do, as I said, I think this is a completely acceptable outcome.

jaebaek · 2020-07-08T20:06:12Z

source/opt/local_single_store_elim_pass.cpp

+          store_inst);
+      context()->get_debug_info_mgr()->KillDebugDeclares(var_id);
+    }
+  }


If I understand it correctly, it works because the var_inst at this point has only a single OpStore.

tl;dr

The following looks correct for me

if (the declare is DebugValue with Deref that does not dominate store_inst) return; else // the logic you wrote

Reasoning

I am sorry for bothering you but I am still considering how we must handle DebugValue with Deref.
I opened a bug #3504. I will update the ssa-rewrite pass for it to consider the dominance relation when we use DebugValue with Deref for the declaration.

For example, we have to consider the dominance relation for the following case:

%x = alloca i32 store i32 13, %x // debugger says %dbg_x = uninitialized store i32 7, %x // debugger says %dbg_x = uninitialized llvm.dbg.value(%x, %dbg_x, DW_OP_deref) store i32 2, %x // debugger says %dbg_x = 2 --> because this store is dominated by the above llvm.dbg.value store i32 4, %x // debugger says %dbg_x = 4

When we remove llvm.dbg.value with DW_OP_deref, we have to propagate llvm.dbg.value for the store i32 2 and store i32 4, but not for the store i32 13 and store i32 7.
This is mainly because llvm.dbg.value is control-dependent while llvm.dbg.declare is not.

Consider the following case

%foo = OpVariable %ptr_Function_int ... OpStore %foo %int_13 // at this point, the compiler must print "undefined" for %dbg_foo ... DebugValue %dbg_foo %foo Deref // from here, the compiler must print the value of %dbg_foo as Deref(%foo)

I think we have to remove the declare and add DebugValue only when the declare is DebugDeclare or it is DebugValue with Deref that dominates the store_inst.

Briefly,

auto *dbg_decl = context()->get_debug_info_mgr()->GetDebugDeclare(var_id); if (dbg_decl == nullptr) return; if (dbg_decl->GetOpenCL100DebugOpCode() != DebugDeclare) { BasicBlock* dbg_decl_block = context()->get_instr_block(dbg_decl); DominatorAnalysis* dominator_analysis = context()->GetDominatorAnalysis(dbg_decl_block->GetParent()); if (!dominator_analysis->Dominates(dbg_decl, store_inst)) return; } // the code you wrote const analysis::Type* var_type = context()->get_type_mgr()->GetType(var_inst->type_id()); const analysis::Type* store_type = var_type->AsPointer()->pointee_type(); if (!(store_type->AsStruct() || store_type->AsArray())) { context()->get_debug_info_mgr()->AddDebugValue( store_inst, var_id, store_inst->GetSingleWordInOperand(1), store_inst); context()->get_debug_info_mgr()->KillDebugDeclares(var_id); }

I think I have a pretty good reason that we don't have to check for dominance. I will write up my thinking tomorrow morning.

Yes, you can hand-write SPIR-V such that a DebugValue/Deref instruction does not dominate a Store to the associated variable, but will it ever happen for valid and useful transformations applied to a valid shader generated for a GLSL or HLSL shader?

Asked another way: is there a legitimate and useful reason that a DebugValue/Deref instruction would not dominate the full live range of its associated variable?

I would propose that the answer is no.

For local variables in shader languages, there is a 1-1 correspondence between a variable and its memory. So there is no reason for a DebugValue/Deref to not dominate all Stores of its associated variable.

I am presuming that any pass that generates a DebugValue/Deref will generate it so that it dominates all its associated Stores and no legitimate subsequent pass will break that. So I don't see a reason to check for that, in the same way we do not need to check for validity of any other code.

In summary, I posit that it is not necessary to check for dominance, here or in ssa-rewrite.

Adding dominance checks would greatly and needlessly increase the complexity of these transformations.

Ok. I agree with you that we would not have the case. Let's revisit it when we have an actual example.
Thank you for reviewing my opinion!

…sGroup#3498) This pass basically follows the same process as ssa-rewrite: it adds a DebugValue after each Store and removes the DebugDeclare or DebugValue Deref. It only does this if all instructions that are dependent on the Store are Loads and are replaced.

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

jaebaek added the component:optimization label Jul 8, 2020

jaebaek reviewed Jul 8, 2020

View reviewed changes

jaebaek approved these changes Jul 9, 2020

View reviewed changes

Preserve OpenCL.DebugInfo.100 through elim-local-single-store

7036f7f

greg-lunarg force-pushed the dbg_store6 branch from 9cf4a6b to 7036f7f Compare July 10, 2020 18:15

jaebaek merged commit cf8c86a into KhronosGroup:master Jul 10, 2020

Conversation

greg-lunarg commented Jul 7, 2020

Uh oh!

greg-lunarg commented Jul 7, 2020

Uh oh!

jaebaek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tl;dr

Reasoning

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants