[PATCH] D80691: Proposed fix for PR46114

[Qirun Zhang via Phabricator via llvm-commits]

helloqirun added a comment.

Hi @Orlando 
Thanks! Yes, if we want to have two `dbg.value` in the IR, it works fine. That's precisely what was before e5f07080b8a <https://reviews.llvm.org/rGe5f07080b8ac73dd573e81a186925a122ab8a39d>, i.e., the final IR will contain

  call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31
  call void @llvm.dbg.value(metadata i32 %0, metadata !22, metadata !DIExpression()), !dbg !31

and lldb can print the correct value of 0.

In e5f07080b8a <https://reviews.llvm.org/rGe5f07080b8ac73dd573e81a186925a122ab8a39d>, we introduced the function `RemoveRedundantDbgInstrs` to eliminate the redundant dbg.value. So now, SimplyCFG will call `RemoveRedundantDbgInstrs` and then `removeRedundantDbgInstrsUsingBackwardScan` to remove the `call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31`.

>From the comment, I see that `RemoveRedundantDbgInstrs` seems to be elegant and it has been used in other locations. For some reasons, `removeRedundantDbgInstrsUsingBackwardScan` sees that the last `dbg.value` is always the latest.

So I see the following options:
(1) Change earlyCSE to make it compatible with the logic in `removeRedundantDbgInstrsUsingBackwardScan`
(2) fix the logic in `removeRedundantDbgInstrsUsingBackwardScan`, but we also need to make sure that it works for all calling contexts
(3) undo the introduction of `RemoveRedundantDbgInstrs`, but we will see duplicate `dbg.value`s

After my initial investigation, I think (1) may be the most promising way to fix this issue.

In D80691#2059463 <https://reviews.llvm.org/D80691#2059463>, @Orlando wrote:

> Hi @helloqirun,
>
> Thanks for working on this!
>
> Assuming your IR is from the code you provided in your reproducer PR46114 [1],
>  compiled with clang at 709c52b9553 <https://reviews.llvm.org/rG709c52b9553f4ba83ab901a873392f8a7f2c56a5> (18 May 2020) I see this:
>
>   $ cat test.c
>   int a = 1;
>   short b;
>   char c;
>   int main() {
>     int d = a, l_52 = 0;
>     b = (l_52 = a) - c; //   qirun0
>     if (d) {
>       int e=1;
>     }
>   }
>  
>   $ clang test.c -O3 -g -mllvm -print-after-all
>   ...
>   *** IR Dump After SROA *** // BEFORE EARLY CSE
>   ; Function Attrs: nounwind uwtable
>   define dso_local i32 @main() #0 !dbg !17 {
>   entry:
>     %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
>     call void @llvm.dbg.value(metadata i32 %0, metadata !21, metadata !DIExpression()), !dbg !31
>     call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31
>     %1 = load i32, i32* @a, align 4, !dbg !32, !tbaa !27
>     call void @llvm.dbg.value(metadata i32 %1, metadata !22, metadata !DIExpression()), !dbg !31
>     %2 = load i8, i8* @c, align 1, !dbg !33, !tbaa !34
>     %conv = sext i8 %2 to i32, !dbg !33
>     ...
>  
>   *** IR Dump After Early CSE ***
>   ; Function Attrs: nounwind uwtable
>   define dso_local i32 @main() #0 !dbg !17 {
>   entry:
>     %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
>     call void @llvm.dbg.value(metadata i32 %0, metadata !21, metadata !DIExpression()), !dbg !31
>     call void @llvm.dbg.value(metadata i32 0, metadata !22, metadata !DIExpression()), !dbg !31
>     call void @llvm.dbg.value(metadata i32 %0, metadata !22, metadata !DIExpression()), !dbg !31
>     %1 = load i8, i8* @c, align 1, !dbg !32, !tbaa !33
>     %conv = sext i8 %1 to i32, !dbg !32
>     ...
>
>
> During EarlyCSE we detect that `%0 = load i32, i32* @a` is the same as 
>  `%1 = load i32, i32* @a`, so the latter is removed. We rewrite the dbg.value
>  which uses %1 to use %0 instead, because %1 is being removed. IIUC this
>  behaviour in EarlyCSE looks correct, and makes me feel like the issue
>  comes from somewhere else?
>
> [1] https://bugs.llvm.org/show_bug.cgi?id=46114





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80691/new/

https://reviews.llvm.org/D80691