llvm dev - Sep 2020 - LSR breaks debug info

The Loop Strength Reduction pass appears to break debug information even for
the most basic input. I believe this is a well known issue already (see
https://bugs.llvm.org/show_bug.cgi?id=38815) but I also believe that it deserve
some extra attention.

Consider the following input compiled with 'clang -g -O3 foo.c -mllvm
-print-after-all'
---
void foo(unsigned char *p) {
#pragma clang loop unroll(disable)
  for (unsigned char i = 0; i < 32; i++) {
    p += 3;
    *p = i;
  }
}
---
As can be seen below in the pre-LSR dump the pointer 'p' is desribed by
dbg.value both before and after it is incremented. In the post-LSR setting
these dbg.value are now referring to undef so the debug-info for 'p' is
effectively lost.

It is interesting to note that after LSR one dbg.value has its DIExpression
adjusted to add the value three but it is still referring to undef. As I
understand it this is the work of salvageDebugInfo. When LSR has created the
new induction variable expression and starts deleting the old one salvaging
will kick in. When %add.ptr is deleted it will adjust the dbg.value use of
that to use the value %p.addr.05, and by doing so it must accordingly adjust the
DIExpression. This all works fine until the phi-node is also deleted at which
point there is not much that can be done with the current scheme.

One idea for how to address this would be that, since LSR is a SCEV based
optimization, one could perform additional debug salvaging by comparing SCEV
expressions for the new and old PHI-node and then adjusting DIExpressions if
they match with an offset.

Any thoughts on that?

-Markus
---
*** IR Dump After Canonicalize Freeze Instructions in Loops ***
; Preheader:
entry:
  call void @llvm.dbg.value(metadata i8* %p, metadata <0x111daf20>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111d4030>)
  call void @llvm.dbg.value(metadata i8 0, metadata <0x111dcb00>, metadata
!DIExpression()), !dbg !DILocation(line: 0, scope: <0x111dc650>)
  br label %for.body, !dbg !DILocation(line: 3, column: 3, scope:
<0x111dc650>)

; Loop:
for.body:                                         ; preds = %entry, %for.body
  %i.06 = phi i8 [ 0, %entry ], [ %inc, %for.body ]
  %p.addr.05 = phi i8* [ %p, %entry ], [ %add.ptr, %for.body ]
  call void @llvm.dbg.value(metadata i8 %i.06, metadata <0x111dcb00>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111dc650>)
  call void @llvm.dbg.value(metadata i8* %p.addr.05, metadata
<0x111daf20>, metadata !DIExpression()), !dbg !DILocation(line: 0, scope:
<0x111d4030>)
  %add.ptr = getelementptr inbounds i8, i8* %p.addr.05, i64 3, !dbg
!DILocation(line: 4, column: 7, scope: <0x111ddc90>)
  call void @llvm.dbg.value(metadata i8* %add.ptr, metadata <0x111daf20>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111d4030>)
  store i8 %i.06, i8* %add.ptr, align 1, !dbg !DILocation(line: 5, column: 8,
scope: <0x111ddc90>), !tbaa <0x111dd288>
  %inc = add nuw nsw i8 %i.06, 1, !dbg !DILocation(line: 3, column: 38, scope:
<0x111dd510>)
  call void @llvm.dbg.value(metadata i8 %inc, metadata <0x111dcb00>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111dc650>)
  %exitcond.not = icmp eq i8 %inc, 32, !dbg !DILocation(line: 3, column: 31,
scope: <0x111dd510>)
  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body, !dbg
!DILocation(line: 3, column: 3, scope: <0x111dc650>), !llvm.loop
<0x111de7b0>

; Exit blocks
for.cond.cleanup:                                 ; preds = %for.body
  ret void, !dbg !DILocation(line: 7, column: 1, scope: <0x111d4030>)
*** IR Dump After Loop Strength Reduction ***
; Preheader:
entry:
  call void @llvm.dbg.value(metadata i8* %p, metadata <0x111daf20>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111d4030>)
 call void @llvm.dbg.value(metadata i8 0, metadata <0x111dcb00>, metadata
!DIExpression()), !dbg !DILocation(line: 0, scope: <0x111dc650>)
  %scevgep = getelementptr i8, i8* %p, i64 3, !dbg !DILocation(line: 3, column:
3, scope: <0x111dc650>)
  br label %for.body, !dbg !DILocation(line: 3, column: 3, scope:
<0x111dc650>)

; Loop:
for.body:                                         ; preds = %entry, %for.body
  %lsr.iv = phi i8* [ %scevgep, %entry ], [ %scevgep7, %for.body ]
  %i.06 = phi i8 [ 0, %entry ], [ %inc, %for.body ]
  call void @llvm.dbg.value(metadata i8 %i.06, metadata <0x111dcb00>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111dc650>)
  call void @llvm.dbg.value(metadata i8* undef, metadata <0x111daf20>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111d4030>)
  call void @llvm.dbg.value(metadata i8* undef, metadata <0x111daf20>,
metadata !DIExpression(DW_OP_plus_uconst, 3, DW_OP_stack_value)), !dbg
!DILocation(line: 0, scope: <0x111d4030>)
  store i8 %i.06, i8* %lsr.iv, align 1, !dbg !DILocation(line: 5, column: 8,
scope: <0x111ddc90>), !tbaa <0x111dd288>
  %inc = add nuw nsw i8 %i.06, 1, !dbg !DILocation(line: 3, column: 38, scope:
<0x111dd510>)
  call void @llvm.dbg.value(metadata i8 %inc, metadata <0x111dcb00>,
metadata !DIExpression()), !dbg !DILocation(line: 0, scope: <0x111dc650>)
  %scevgep7 = getelementptr i8, i8* %lsr.iv, i64 3, !dbg !DILocation(line: 3,
column: 31, scope: <0x111dd510>)
  %exitcond.not = icmp eq i8 %inc, 32, !dbg !DILocation(line: 3, column: 31,
scope: <0x111dd510>)
  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body, !dbg
!DILocation(line: 3, column: 3, scope: <0x111dc650>), !llvm.loop
<0x111de7b0>

; Exit blocks
for.cond.cleanup:                                 ; preds = %for.body
  ret void, !dbg !DILocation(line: 7, column: 1, scope: <0x111d4030>)
---
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200910/146c3e5b/attachment.html>

Hi Markus,

On Thu, Sep 10, 2020 at 8:44 AM Markus Lavin <markus.lavin at
ericsson.com> wrote:
> The Loop Strength Reduction pass appears to break debug information even
for
> the most basic input. I believe this is a well known issue already (see
> https://bugs.llvm.org/show_bug.cgi?id=38815) but I also believe that it
deserve
> some extra attention.
Indeed, it's a poor performer, and losing loop variables is one of the
most common complaints I've heard,

[...]
> One idea for how to address this would be that, since LSR is a SCEV based
> optimization, one could perform additional debug salvaging by comparing
SCEV
> expressions for the new and old PHI-node and then adjusting DIExpressions
if
> they match with an offset.
>
> Any thoughts on that?
It sounds like a plan -- I'm not especially familiar with SCEV, but if
we can determine equivalence in that manner then that kind of
salvaging would be sound. It feels like it would cover quite a common
case, and would be an "easy" win.

How difficult is it to identify an offset in SCEV? It's probably OK to
just compare expressions, as offsets such as in your example can be
recovered with the existing salvageDebugInfo function. It wouldn't be
worth putting a lot of effort into /interpreting/ SCEV expressions, as
we may as well go all the way and ask SCEV to produce an expression
for each dbg.value, and implement the general solution. That's what
I'd like to happen in the long term, but this sounds like a great
stepping stone along the way.

--
Thanks,
Jeremy

Hi Jeremy, 
> It sounds like a plan -- I'm not especially familiar with SCEV, but if
we can
> determine equivalence in that manner then that kind of salvaging would be
> sound. It feels like it would cover quite a common case, and would be an
> "easy" win.
I am not very familiar with IR SCEV (or the LSR code) but in our downstream
target we have a similar pass to LSR that operates on MIR and uses a SCEV
implementation for our MIR. There we face almost exactly the same problem of how
to keep the DBG_VALUEs up to date after an induction variable has been replaced
with a newly generated instruction sequence (including PHI). In that setting
(which is likely much simplified compared to IR SCEV and LSR) I have played a
bit with comparing SCEV expressions and compensating for potential offsets with
DIExpressions and there that seems like a viable solution.

> How difficult is it to identify an offset in SCEV? It's probably OK to
just
> compare expressions, as offsets such as in your example can be recovered
> with the existing salvageDebugInfo function. It wouldn't be worth
putting a
> lot of effort into /interpreting/ SCEV expressions, as we may as well go
all the
> way and ask SCEV to produce an expression for each dbg.value, and
> implement the general solution. That's what I'd like to happen in
the long
> term, but this sounds like a great stepping stone along the way.
For the IR I posted earlier I believe the SCEVs would be

Before LSR: %p.addr.05 {%p,+,3}
After LSR: %lsr.iv {3+%p,+,3}

So I do not think it would be very difficult to extract these offsets but then
again I am not really familiar with IR SCEV.
I will try to look a round a bit in LSR and see if I can come up with something.

Thanks
Markus