I think we have to allow OpenCLDebugInfo100DebugDeclare as well.
See the following example that has DebugValue referencing a local variable pointer:

%deref = OpExtInst %void %ext DebugOperation Deref
%deref_expr = OpExtInst %void %ext DebugExpression %deref
...
%foo = OpVariable %ptr_Function_int Function
...
OpStore %foo %int_13
%val = OpExtInst %void %ext DebugValue %dbg_foo %foo %deref_expr

@greg-lunarg I believe that jaebaek is right. You should look for a DebugValue here as well. Scalar replacement will be replacing DebugDeclare instructions with DebugValue instruction as shown above. This would still be a supported ref.

In my opinion, we should allow delete store for this case. Consider the following example:

%empty_expr = OpExtInst %void %ext DebugExpression
...
%foo = OpVariable %ptr_Function_int Function
...
OpStore %foo %int_13  // will be deleted
OpStore %foo %int_17
%decl = OpExtInst %void %ext DebugDeclare %dbg_foo %foo %empty_expr

We have to delete OpStore %foo %int_13.

The following case looks non-trivial:

%empty_expr = OpExtInst %void %ext DebugExpression
...
%foo = OpVariable %ptr_Function_int Function
...
OpStore %foo %int_13  // will be deleted
%decl = OpExtInst %void %ext DebugDeclare %dbg_foo %foo %empty_expr
...
OpStore %foo %int_17

IMO, we have to change it to

%empty_expr = OpExtInst %void %ext DebugExpression
...
%foo = OpVariable %ptr_Function_int Function
...
%val = OpExtInst %void %ext DebugValue %dbg_foo %int_13 %empty_expr
...
OpStore %foo %int_17
%decl = OpExtInst %void %ext DebugDeclare %dbg_foo %foo %empty_expr

What do you think?

I think we should just not delete the store for any variable (scalars as well) that have a debug declare. If DebugDeclare is suppose to match llvm.dbg.declare, the address has to always contains the correct value. However, the spir-v extension is not as clear as the llvm-ir spec, this might be fine.

I think that as long as we still remove the loads, this pass will still do what it needs to do.

For clarification, do you believe that we should not remove the store even if we replace it with a DebugValue, which is being done here?

More generally, can we allow DebugDeclare and DebugValue to coexist for the same DebugLocalVariable? I think we can temporarily within spirv-opt as long as we ultimately remove the DebugDeclares before outputting the SPIR-V.

I don't think we should remove the store. The DebugValue is not needed, but I don't think it hurts.

can we allow DebugDeclare and DebugValue to coexist for the same DebugLocalVariable?

The spec is so unclear, I am not sure what we can or cannot do. I believe it would be okay to have both as long as they give the same information.

I think we can temporarily within spirv-opt as long as we ultimately remove the DebugDeclares before outputting the SPIR-V.

If we could guarantee that users always run ssa-rewrite then I could agree with this: temporarily invalid code is fine. However, our passes are meant to be used in potentially any order. We have tried to ensure that no pass requires another pass to run in order to be correct. The only exception so far is merge-return and dead-branch elimination. In that case an error is emitted (https://github.com/KhronosGroup/SPIRV-Tools/blob/master/source/opt/merge_return_pass.cpp#L108).

Ah ok, my comment was wrong.
llvm-dbg-declare explains "It is not control-dependent ... that address is considered to be the true home of the variable across its entire lifetime."

We must not delete the store if the variable is used for DebugDeclare.

Can/should the legalization passes do less optimization in the presense of OpenCL.DebugInfo.100? For example, can/should the passes not eliminate OpStores? I think it is possible to eliminate OpStores as long as we replace them with DebugValues.

If the OpVariable is used by a DebugDeclare or a DebugValue then we should not remove the store unless you can also remove the use in the DebugDeclare or DebugValue. ssa-rewrite if removing the DebugDeclare and DebugValue, so later passes like ADCE can remove the stores. We can still remove the loads at any time, and do the copy propagation etc.

I think it might be possible to legalize without removing OpStores. I will have to go back to our original legalization shaders and see. I just thought, to be safe, we should try to stick to the original passes as closely as possible but still retain the debug information.

I believe so as well, as long at the loads are removed. That is what we really wanted from elim-local-single-block and elim-local-single-store anyway.

I am asking because I am looking at DCE right now which eliminates a lot of OpStores. If you think that is bad, it would be good to know now.

As I said before DCE should not remove the store if the OpVariable is used in a DebugDeclare or DebugValue. The ssa rewriter should remove those uses in cases where we can remove the stores, so I don't think it will hinder optimization.

I think it is ok to replace a single OpStore with a DebugValue as long as we eventually replace them all and get rid of the DebugDeclare before outputting the SPIR-V.

I agree with the general philosophy that each pass should be able to stand alone, but elim-local-single-block is not really a stand-alone pass; it was written to be a helper to ssa-rewrite, so I think it is ok to assume that ssa-rewrite will follow.

That said, I don't think its a huge deal if we don't eliminate OpStores in elim-local-single-block; follow up passes will take care of them. I was just trying to create as little new logic and exceptions and different behavior for DebugInfo as possible in the interest of reducing complexity.

OK. Just looked at output from ssa-rewrite. Now I understand your approach better.

ssa-rewrite is adding DebugValue after each Store and removing the DebugDeclare. So essentially ssa-rewrite is preparing the OpStores and OpVariable for deletion.

I can work with this model now that I understand it.

@greg-lunarg I believe that jaebaek is right. You should look for a DebugValue here as well. Scalar replacement will be replacing DebugDeclare instructions with DebugValue instruction as shown above. This would still be a supported ref.

This is a good test. We should add another one where a DebugValue with a dref expression is used in place of the DebugDeclare. We would expect the same resutls.

I admit that the OpenCL.DebugInfo.100 spec is vague, but after some research and thought, I do not believe that a DebugValue with a deref of a pointer Value is the same as a DebugDeclare. A DebugValue represents an assignment of the Value and Expression to the rvalue of the LocalVariable. A DebugDeclare represents an "equivalencing" of the Expression applied to lvalue of the Variable to the lvalue of the LocalVariable. I am fairly certain these are not the same thing.

I am also fairly certain that we will never have cause to have a deref in an Expression in a DebugValue in a shader. The Variable would have to be of type pointer into function scope memory which don't exist in shaders.

After some thought, while I am still fairly certain that one cannot use a DebugValue to create an equivalent to a DebugDeclare in a shader, I will grant that one could create a legitimate DebugValue that contains a deref of a local pointer for a shader, However, I think that such a reference would be so highly unlikely that it would not be worth creating the logic to add it to the supported references for this optimization. If such a thing did appear, it would merely disable optimization and not generate incorrect code. So I think the existing logic is reasonable and acceptable at this time. If someone can make an argument for why such a construct would be prevalent enough to justify optimization here, we can reconsider this logic at that time.

A DebugValue represents an assignment of the Value and Expression to the rvalue of the LocalVariable.

I have thought about this a lot as well when trying to figure out the design for scalar replacement. I understand your point of view. At first, I was thinking that way as well. However, I started thinking about how this would actually be implemented by a debugger, and in dwarf. In DWARF, you don't give the value of a variable. DWARF say where you go to get the value (register or memory). The important point to realize is that the DebugValue is not an assignment of a value. This can be easily confused when rerefs are not used because, in llvm-ir and in spir-v, the ssa value cannot be changed.

The other reason that I believed the llvm.dbg.value could not be viewed as an assignment of a value is the extra work this would put on debuggers, and how it would slow down debugging. The debugging info is not present in the machine code. If the debugger was required to capture the value of the expression at every llvm.dbg.value, the debugger would have to stop the normal execution of the program at every position of an llvm.dbg.value to read the value. This would cause a ridiculous amount of overhead in debugging. We wouldn't want a spir-v debugger to have to do the same.

Granted this was just guesses, which is why I ask Jaebaek to look at what code actually gets generated by llvm-ir when a debug value is used with a dref expression, and to examine what a real debugger does with it. We hand wrote llvm-ir that has exactly one llvm.dbg.value where the address of the memory location is the value and there is a deref expression. The instruction was placed after the first store to the memory location. There was a second store to the same memory location, but there was not a second llvm.dbg.value. When running in the debugger, you see the value of the first store when you break after the first store. However, when you break after the second store, you see the result of the second store. The debugger did not capture the value of the first store to be used anytime the developer asks for the value of the variable. The debugger checks the value in the memory location.

As Steven mentioned, we confirmed that GDB can track a variable decorated by llvm.dbg.value with deref similar to a variable decorated by llvm.dbg.declare.
Since I am also considering how I should understand the "debug value", I recently conducted one more experiment, in particular, to check "llvm.dbg.declare is not control-dependent" (See llvm-dbg-declare).

I moved llvm.dbg.declare and print the variable value in GDB. It is independent on its position but it depends on its scope. If it is under a function, we can print the value of the variable anytime in the function even when I moved it to the end of the function.

I tested the same thing using llvm.dbg.value, but it is control-dependent. As far as I understand, it is the only difference between debug value with deref and debug declare in the view of GDB (and the DWARF section generated by the LLVM IR. I checked the X86 assembly code).
I am still considering how it affects on our implementation. The main concern is how to keep the position of debug value in DebugInfoManager and how to handle reference variables or pointer-to-pointer replacements, but for the this unit test, I agree with Steven that it would be better to add a test simulating the case we use DebugValue with Deref instead of DebugDeclare.

OK. Thanks for the detailed response. I do now agree that a DebugDeclare can be replaced with a DebugValue with a deref, and I apologize that it took me this long to come to this understanding.

So I guess now the question in my mind is: is this really what we want to do?

Essentially, to do this, we have to keep backing memory for all locals (function scope variables) and keep all Stores to that memory that we normally remove. So we will have to add extra logic to all optimizations that eliminate stores. In one way it will be simpler for the optimizer since it only has to generate one DebugValue at the top of the program, but there will be performance cost from the extra Stores.

This design will also require a whole additional and redundant memory analysis of derefs in DebugValues.

The next question is: do we need to do this? You opined above that DebugValue would be intractable for debuggers as I imagined them. But if you look at https://llvm.org/docs/SourceLevelDebugging.html#object-lifetime-in-optimized-code it appears that llvm.dbg.value is being used in just this manner.

As far as debugger implementation, I imagine the debugger would be able to create a fairly simple map for each local variable, mapping a line number range to a register.

Still, your design will functionally work, albeit with probable performance costs, and I can work with it.

I think I can just look at the Value field of the DebugValue and see if its a pointer, so that should be fairly easy.

Your questions are very important questions to remove the technical debt and make a reasonable design. I appreciate it. I am conducting more experiments to figure out the relationship between llvm.dbg.declare and llvm.dbg.value particularly with DW_OP_LLVM_fragment.

I am sorry for delaying approval for this PR long time but I suspect using DebugValue with Deref for the declare. I want to take more time to consider this issue before going further.

IIRC, we decided to use DebugValue with Deref to express the declaration because we thought it is the only way to express the declaration in the scalar replacement. In LLVM world, SROA conducts both the scalar replacement and ssa-rewrite in a single pass, so it does not have to consider "splitting a composite type while keeping the information of declaration". On the other hand, spirv-opt scalar replacement must "split a composite type while keeping the information of declaration". Since DebugDeclare does not have Indexes operand while DebugValue does, we had no idea how to use DebugDeclare for each member of a composite. See DebugDeclare.

The main questions I currently have are

• Q1: Does LLVM SROA pass propagate llvm.dbg.value for store and phi instructions when we give llvm.dbg.value with DW_OP_deref?
• Q2: Can we use DW_OP_LLVM_fragment with llvm.dbg.declare for the scalar replacement?
• Q3: Is there a way to express a member of a composite using DebugDeclare and Deref operation?

If Q2 and Q3 are true, I think we do not have to use DebugValue with Deref for the declaration in the scalar replacement.
In addition, if Q2 and Q3 are true and Q1 is false, it would be better to follow what LLVM does i.e., we must not use DebugValue with Deref as a declaration.

In my brief experiments, Q1 is false and Q2 is true, but I am doublechecking it if I made some mistakes. I am also digging into LLVM source code.
As far as I understand, DW_OP_LLVM_fragment is applied to the variable not its value of llvm.dbg.value nor its storage of llvm.dbg.declare.
I am curious what DW_OP_bit_piece means because I guess (I do not have any evidence yet) that it is applied to the value of llvm.dbg.value or the storage of llvm.dbg.declare.

For Q3, I am discussing the spec change with the initial author of the spec.

Please give me 1 or 2 more days. I will figure them out and let you know.