[PATCH] D80691: Proposed fix for PR46114

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

This is quite a long reply so I've got a very small "summary" at the end.

Asking for the final output (looking at the dwarf) was possibly counterproductive because other
transformations may have an impact and misdirect our attention. Looking at the pass alone will be
helpful; the test should provide a good reference point for this discussion.

> If we do RAUW first, the corresponding value will be replaced -- that causes PR46114. With this
>  patch, we set it as undef. If I missed anything, please let me know.

What I was trying to say is that I believe that the RAUW is correct, and that it is not the cause of
PR46114. Ignoring PR46114 and focusing only on this patch for now, using the IR from your test, I'll
try to explain my reasoning for why the RAUW is correct and how your patch reduces variable location
availability:

  Before EarlyCSE:
  %0 = load i32, i32* @a, align 4
  
  ; Value of "l_52" is 0.
  call void @llvm.dbg.value(metadata i32 0, metadata !20, metadata !DIExpression()), !dbg !24
  
  %1 = load i32, i32* @a, align 4, !dbg !25, !tbaa !26
  
  ; Value of "l_52" is found in virtual register %1.
  call void @llvm.dbg.value(metadata i32 %1, metadata !20, metadata !DIExpression()), !dbg !24
  
  %sub = sub nsw i32 %1, 0
  
  !20 = !DILocalVariable(name: "l_52",

EarlyCSE detects that the loads into %0 and %1 are exactly the same, so we can remove the later one,
%1. Without your patch, we replace all uses (including debug uses) with %0, because we've proven
that %0 and %1 have the same value.

  After EarlyCSE:
  %0 = load i32, i32* @a, align 4
  
  ; Value of "l_52" is 0.
  call void @llvm.dbg.value(metadata i32 0, metadata !20, metadata !DIExpression()), !dbg !24
  
  ; This has been removed by EarlyCSE.
  ; %1 = load i32, i32* @a, align 4, !dbg !25, !tbaa !26
  
  ; Value of "l_52" is now found in virtual register %0, previously %1.
  call void @llvm.dbg.value(metadata i32 %0, metadata !20, metadata !DIExpression()), !dbg !24
  
  ; Sub now uses %0 instead of %1, because %1 has been deleted and they were equivalent.
  %sub = sub nsw i32 %0, 0
  
  !20 = !DILocalVariable(name: "l_52",

And again, without my comments, before RemoveRedundantDbgInstrs runs.

  After EarlyCSE:
  %0 = load i32, i32* @a, align 4
  call void @llvm.dbg.value(metadata i32 0, metadata !20, metadata !DIExpression()), !dbg !24
  call void @llvm.dbg.value(metadata i32 %0, metadata !20, metadata !DIExpression()), !dbg !24
  %sub = sub nsw i32 %0, 0
  
  !20 = !DILocalVariable(name: "l_52",

Notice that the dbg.values are adjacent in the IR and refer to the same variable ("l_52"). The first
says "after '%0 = load ...' the value of l_52 is 0". The second says "after '%0 = load ...' the
value of l_52 is in virtual register %0". It is safe to remove the first one because the second
renders it redundant. This is what RemoveRedundantDbgInstrs does:

  After RemoveRedundantDbgInstrs:
  %0 = load i32, i32* @a, align 4
  ; The following dbg.value has been removed.
  ; call void @llvm.dbg.value(metadata i32 0, metadata !20, metadata !DIExpression()), !dbg !24
  call void @llvm.dbg.value(metadata i32 %0, metadata !20, metadata !DIExpression()), !dbg !24
  %sub = sub nsw i32 %0, 0
  
  !20 = !DILocalVariable(name: "l_52",

The meaning of the debug-info remains unchanged. We're still saying "after '%0 = load ...' the
value of l_52 is in virtual register %0".

With your patch, we instead see this after EarlyCSE:

  After EarlyCSE with your patch:
  %0 = load i32, i32* @a, align 4
  call void @llvm.dbg.value(metadata i32 0, metadata !20, metadata !DIExpression()), !dbg !24
  call void @llvm.dbg.value(metadata i32 undef metadata !20, metadata !DIExpression()), !dbg !24
  %sub = sub nsw i32 %0, 0
  
  !20 = !DILocalVariable(name: "l_52",

'undef' dbg.value means that we can't provide a variable location from here until the next
dbg.value. In this example there is no next dbg.value, so we are saying that we cannot provide the
value of "l_52" at any point in the program. However, hopefully I've been able to illustrate that %0
is a valid variable location for l_52 at this point in the IR.

Summary:
Just to be clear; I agree that the behaviour you show in the ticket PR46114 [1] is a bug, I just
think that this patch covers up the problem by reducing variable location coverage, instead of
fixing it. I've written up a possible cause of the bug on the ticket itself.

I hope this helps, please let me know if I've been unclear anywhere.

[1] https://bugs.llvm.org/show_bug.cgi?id=46114


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

https://reviews.llvm.org/D80691