Preserve debug info in scalar replacement pass by jaebaek · Pull Request #3461 · KhronosGroup/SPIRV-Tools

jaebaek · 2020-06-25T18:12:54Z

Preserve OpenCL.DebugInfo.100 and line information in scalar
replacement pass.

Set the debug scope and line information for the new replacement
instructions.
Replace DebugDeclare and DebugValue if their OpVariable or value
operands are replaced by scalars.

If the initial aggregate has DebugValue, we clone a DebugValue
for each scalar and add one more index for the scalar to 'Indexes'
operand of the cloned DebugValue as we add an index to 'Indexes'
operand of OpAccessChain.
If the initial aggregate has DebugDeclare, we create a DebugValue
with Deref operation for each scalar and add an index for the scalar
to 'Indexes' operand.
For example,

   struct S { int a; int b;}
   S foo; // before scalar replacement

   int foo_a; // after scalar replacement
   int foo_b;

   DebugDeclare %dbg_foo %foo %null_expr // before

   DebugValue %dbg_foo %foo_a %Deref_expr 0 // after
   DebugValue %dbg_foo %foo_b %Deref_expr 1 // means Value(foo.members[1]) == Deref(%foo_b)

Note that the DebugValue instructions for the SROA'd structure
elements are linked to the original debug info for S foo via the
%dbg_foo. Each DebugValue has Deref and 'Indexes' i.e., 0
for %foo_a and 1 for %foo_b in the above example.
DebugValue %dbg_foo %foo_a %Deref_expr 0 means that
Deref(%foo_a) is the value of 0th member of %dbg_foo.

jaebaek · 2020-06-25T18:13:47Z

FYI, @greg-lunarg This is the PR for the scalar replacement.

jaebaek · 2020-07-06T22:11:10Z

We currently discuss some SPIR-V spec issues related to this PR. I will update this PR after that.

dnovillo

Some initial questions. Still haven't read through the whole thing.

dnovillo · 2020-07-15T20:07:52Z

source/opt/constants.h

  // Returns the id of a 32-bit floating point constant with value |val|.
  uint32_t GetFloatConst(float val);

+  // Returns the id of a 32-bit singed integer constant with value |val|.


s/singed/signed/

dnovillo · 2020-07-15T20:11:40Z

source/opt/debug_info_manager.cpp

+                                              uint32_t expr_id,
+                                              uint32_t index_id,
+                                              Instruction* insert_before) {
+  std::unique_ptr<Instruction> new_dbg_value(new Instruction(


Why doesn't this re-use AddDebugValue? Shouldn't it need to check the same things that AddDebugValue checks? And now in #3511, AddDebugValue returns a boolean flag indicating success. It seems odd that this would not be also expected in this function.

AddDebugValueWithIndex() simply generates and inserts a single DebugValue while AddDebugValue() does the same thing for the multiple DebugValue. I updated AddDebugValue() to use AddDebugValueWithIndex().

In terms of the boolean return of AddDebugValue(), it does not matter that much of this PR. It will follow #3511 when I merge it.

OK, so now I have a question about AddDebugValue(). It seems to be a low-level primitive, and yet it may (under some conditions) not actually add a DebugValue(). This is confusing, as it would mean that a low-level helper decides whether or not to do something (despite the name of the function). Perhaps rename the function to reflect this? Something like MaybeAdd... or (better) make the caller responsible for calling AddDebugValue when it really means to add a value.

I agree that these names are confusing. I just update AddDebugValue() to AddDebugValueIfVarDeclIsVisible().

jaebaek

Thank you for your code review. I updated it once. PTAL!

jaebaek · 2020-07-16T20:25:57Z

source/opt/constants.h

  // Returns the id of a 32-bit floating point constant with value |val|.
  uint32_t GetFloatConst(float val);

+  // Returns the id of a 32-bit singed integer constant with value |val|.


jaebaek · 2020-07-16T20:28:53Z

source/opt/debug_info_manager.cpp

+                                              uint32_t expr_id,
+                                              uint32_t index_id,
+                                              Instruction* insert_before) {
+  std::unique_ptr<Instruction> new_dbg_value(new Instruction(


AddDebugValueWithIndex() simply generates and inserts a single DebugValue while AddDebugValue() does the same thing for the multiple DebugValue. I updated AddDebugValue() to use AddDebugValueWithIndex().

In terms of the boolean return of AddDebugValue(), it does not matter that much of this PR. It will follow #3511 when I merge it.

dnovillo · 2020-07-17T14:37:53Z

Preserve OpenCL.DebugInfo.100 and line information in scalar
replacement pass.

Set the debug scope and line information for the new replacement
instructions.

Replace DebugDeclare and DebugValue if their OpVariable or value
operands are replaced by scalars. It uses 'Indexes' operand of
DebugValue. For example,
struct S { int a; int b;}
S foo; // before scalar replacement
int foo_a; // after scalar replacement
int foo_b;
DebugDeclare %dbg_foo %foo %null_expr // before
DebugValue %dbg_foo %foo_a %Deref_expr 0 // after
DebugValue %dbg_foo %foo_b %Deref_expr 1 // means Value(foo.members[1]) == Deref(%foo_b)

Add some commentary on why we have to replace DebugDeclare with DebugValue? My understanding from reading https://www.khronos.org/registry/spir-v/specs/unified1/OpenCL.DebugInfo.100.html#DebugDeclare is used to link debug information to a local declaration. When we scalarize the variable, this declaration disappears because the symbol foo disappears. What is not completely clear in this description, is how will a debugger request for foo.a be linked to the dbg_foo debug symbol.

I would add something along the lines of "the DebugValue expressions for the SROA'd structure elements are linked to the original debug info for foo via the dbg_foo symbol. Each DebugValue will have instructions on what field is being accessed via the Deref_* expressions."

Does that sound like I'm understanding the difference between the two?

dnovillo · 2020-07-17T15:04:57Z

source/opt/debug_info_manager.cpp

+                                              uint32_t expr_id,
+                                              uint32_t index_id,
+                                              Instruction* insert_before) {
+  std::unique_ptr<Instruction> new_dbg_value(new Instruction(


OK, so now I have a question about AddDebugValue(). It seems to be a low-level primitive, and yet it may (under some conditions) not actually add a DebugValue(). This is confusing, as it would mean that a low-level helper decides whether or not to do something (despite the name of the function). Perhaps rename the function to reflect this? Something like MaybeAdd... or (better) make the caller responsible for calling AddDebugValue when it really means to add a value.

jaebaek

Thank you for your comments. I think you correctly understand it.
I update the description with more details.
PTAL!

jaebaek · 2020-07-17T19:14:03Z

source/opt/debug_info_manager.cpp

+                                              uint32_t expr_id,
+                                              uint32_t index_id,
+                                              Instruction* insert_before) {
+  std::unique_ptr<Instruction> new_dbg_value(new Instruction(


I agree that these names are confusing. I just update AddDebugValue() to AddDebugValueIfVarDeclIsVisible().

dnovillo

It looks fine to me, but I defer to @s-perron for final approval, as I'm less familiar with this code.

s-perron

One small change, then this is good to merge.

source/opt/scalar_replacement_pass.cpp

1. Set the debug scope and line information for the new replacement instructions. 2. Replace DebugDeclare and DebugValue if their OpVariable or value operands are replaced by scalars. It uses 'Indexes' operand of DebugValue. For example, struct S { int a; int b;} S foo; // before scalar replacement int foo_a; // after scalar replacement int foo_b; DebugDeclare %dbg_foo %foo %null_expr // before DebugValue %dbg_foo %foo_a %Deref_expr 0 // after DebugValue %dbg_foo %foo_b %Deref_expr 1 // means Value(foo.members[1]) == Deref(%foo_b)

jaebaek

Thanks!

source/opt/scalar_replacement_pass.cpp

1. Set the debug scope and line information for the new replacement instructions. 2. Replace DebugDeclare and DebugValue if their OpVariable or value operands are replaced by scalars. It uses 'Indexes' operand of DebugValue. For example, struct S { int a; int b;} S foo; // before scalar replacement int foo_a; // after scalar replacement int foo_b; DebugDeclare %dbg_foo %foo %null_expr // before DebugValue %dbg_foo %foo_a %Deref_expr 0 // after DebugValue %dbg_foo %foo_b %Deref_expr 1 // means Value(foo.members[1]) == Deref(%foo_b)

* Rolling 2 dependencies and updating expectations Roll third_party/spirv-cross/ 6575e451f..0376576d2 (5 commits) KhronosGroup/SPIRV-Cross@6575e45...0376576 $ git log 6575e451f..0376576d2 --date=short --no-merges --format='%ad %ae %s' 2020-07-22 tommek Enabling setting a fixed sampleMask in Metal fragment shaders. 2020-02-20 cdavis MSL: Add support for processing more than one patch per workgroup. 2020-07-22 dsinclair Roll GLSLang, SPIRV-Headers and SPIRV-Tools. 2020-07-22 cdavis MSL: Factor creating a uint type into its own method. 2020-07-22 cdavis MSL: Factor a really gnarly condition into its own method. Created with: roll-dep third_party/spirv-cross Roll third_party/spirv-tools/ 969f028..b63f0e5 (13 commits) KhronosGroup/SPIRV-Tools@969f028...b63f0e5 $ git log 969f028..b63f0e5 --date=short --no-merges --format='%ad %ae %s' 2020-07-27 rdb Fix SyntaxWarning in Python 3.8 (KhronosGroup#3388) 2020-07-27 bclayton CMake: Enable building with BUILD_SHARED_LIBS=1 (KhronosGroup#3490) 2020-07-27 dneto Avoid operand type range checks (KhronosGroup#3379) 2020-07-27 jaebaek Preserve debug info in scalar replacement pass (KhronosGroup#3461) 2020-07-27 pierremoreau Update OpenCL capabilities validation (KhronosGroup#3149) 2020-07-27 stevenperron build(deps): bump lodash from 4.17.15 to 4.17.19 in /tools/sva (KhronosGroup#3596) 2020-07-27 antonikarp spirv-fuzz: adds TransformationReplaceLoadStoreWithCopyMemory (KhronosGroup#3586) 2020-07-27 jaebaek Preserve OpenCL.DebugInfo.100 through private-to-local pass (KhronosGroup#3571) 2020-07-27 stefanomil spirv-fuzz: Relax type checking for int contants (KhronosGroup#3573) 2020-07-27 stefanomil spirv-fuzz: Generalise transformation access chain (KhronosGroup#3546) 2020-07-27 stefanomil spirv-fuzz: Split blocks starting with OpPhi before trying to outline (KhronosGroup#3581) 2020-07-27 afdx spirv-fuzz: Set message consumer in replayer when shrinking (KhronosGroup#3591) 2020-07-24 vasniktel spirv-fuzz: Don't use default parameters (KhronosGroup#3583) Created with: roll-dep third_party/spirv-tools * Add in missing expectations update

jaebaek added the component:optimization label Jun 25, 2020

jaebaek requested review from dnovillo and s-perron June 25, 2020 18:12

jaebaek self-assigned this Jun 25, 2020

jaebaek force-pushed the dbg_scalar_replacement branch from 84cc461 to d45f950 Compare July 15, 2020 17:54

dnovillo requested changes Jul 15, 2020

View reviewed changes

jaebaek commented Jul 16, 2020

View reviewed changes

dnovillo requested changes Jul 17, 2020

View reviewed changes

dnovillo mentioned this pull request Jul 17, 2020

Remove DebugDeclare only for target variables in ssa-rewrite #3511

Merged

jaebaek commented Jul 17, 2020

View reviewed changes

dnovillo reviewed Jul 20, 2020

View reviewed changes

jaebaek force-pushed the dbg_scalar_replacement branch from ebb3182 to a1678d0 Compare July 22, 2020 19:23

s-perron approved these changes Jul 27, 2020

View reviewed changes

source/opt/scalar_replacement_pass.cpp Outdated Show resolved Hide resolved

jaebaek added 9 commits July 27, 2020 10:32

Fix build failure

aa73bd8

Refactoring

3fece57

Fix build failure

fdf7c41

Update based on code review

a2e9f9d

Update based on code review

a01611b

Insert DebugValue with Deref into the first block of function

91fa9c4

Update wrong description

98e65d8

Make function call readable

71373ab

jaebaek force-pushed the dbg_scalar_replacement branch from 7176ca0 to 71373ab Compare July 27, 2020 14:44

jaebaek commented Jul 27, 2020

View reviewed changes

source/opt/scalar_replacement_pass.cpp Outdated Show resolved Hide resolved

s-perron merged commit 6a3eb67 into KhronosGroup:master Jul 27, 2020

jaebaek deleted the dbg_scalar_replacement branch July 27, 2020 17:52

jaebaek mentioned this pull request Jul 27, 2020

OpenCL.DebugInfo.100 information preservation in scalar-replacement pass #3393

Closed

Conversation

jaebaek commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaebaek commented Jun 25, 2020

Uh oh!

jaebaek commented Jul 6, 2020

Uh oh!

dnovillo 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

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!

dnovillo commented Jul 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaebaek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnovillo left a comment

Choose a reason for hiding this comment

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaebaek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaebaek commented Jun 25, 2020 •

edited

Loading