[PATCH] D50621: [DebugInfo] Fix bug in LiveDebugVariables.

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai marked an inline comment as done.
HsiangKai added inline comments.


================
Comment at: lib/CodeGen/LiveDebugVariables.cpp:601
       // Handle consecutive DBG_VALUE instructions with the same slot index.
       do {
         if (handleDebugValue(*MBBI, Idx)) {
----------------
bjope wrote:
> Is the DBG_LABEL always placed before a set of DBG_VALUE instructions, or can they be interleaved?
> 
> Earlier this loop was needed since we did not skip debug instructions when searching backwards for the SlotIndex. With the change above I guess the result would be the same even without the loop. If we for example can have 
> ```
>   DBG_VALUE
>   DBG_VALUE
>   DBG_LABEL
>   DBG_VALUE
>   DBG_VALUE
> ```
> nowadays, then I think that we
> 
> 1) either should not have a loop here
> 
> 2) or we should skip DBG_LABEL instructions in the inner loop
> 
> 3) or we should add some code comments that describes why we need this nested loop (is it still needed as an optimization to avoid doing the SlotIndex lookup multiple times for consecutive DGB_VALUE instructions?)
> 
> 4) alternative (2) plus, the condition on line 581 should be changed from checking isDebugValue() to checking isDebugInstr(). Then the new code that you have added can be removed, and instead the inner loop should call handleDebugValue only for DBG_VALUE instructions, while simply skipping past DBG_LABEL instructions. That way we let the inner loop handle all kinds of consecutive debug instructions that share the same SlotIndex.
> 
> I put my vote on option (4) as I think it is simpler to understand, we do not need backward skipping when having interleaved DBG_LABEL/DBG_VALUE, and we do not need to benchmark to understand if the inner loop is needed just as an optimization or not.
I agree with you. I modified my patch according to your suggestions. It is more clear and easy to understand. I add more comments in the function to show the intension.


https://reviews.llvm.org/D50621