[PATCH] D80691: Proposed fix for PR46114

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

Orlando added subscribers: jmorse, probinson.
Orlando added a comment.

In D80691#2067707 <https://reviews.llvm.org/D80691#2067707>, @helloqirun wrote:

> Thanks @Orlando, I think I agree with your reasoning. My patch on introducing an undef is based on the same reasoning (but I could be totally wrong).
>
> Let's see what we should do instead.
>
> Before earlycse, we have:
>
>     %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
>     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
>  
>   !26 = !DILocation(line: 5, column: 11, scope: !17)
>   !31 = !DILocation(line: 0, scope: !17)
>   !32 = !DILocation(line: 6, column: 15, scope: !17)
>
>
> after earlycse, we have:
>
>     %0 = load i32, i32* @a, align 4, !dbg !26, !tbaa !27
>   ; I omitted the variable !21 here since it's irrelevant.
>     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
>  
>   !21 = !DILocalVariable(name: "d", scope: !17, file: !3, line: 5, type: !12)
>   !22 = !DILocalVariable(name: "l_52", scope: !17, file: !3, line: 5, type: !12)
>   !26 = !DILocation(line: 5, column: 11, scope: !17)
>   !31 = !DILocation(line: 0, scope: !17)
>   !32 = !DILocation(line: 6, column: 20, scope: !17)
>
>
> Could you please help me verify the following?
>  Based on my understanding, it seems that when eliminating the second  `load i32, i32* @a` in EarlyCSE, we should use the location `!32` instead of `!26` for the resulting `load i32, i32* @a`.
>  Thanks.


After EarlyCSE it may be wrong to say that the load `%0 = load ...` comes from either line 5 (!26)
or line 6 (!32) because it now comes from source code from both lines. There is a method to handle
this case called `Instruction::applyMergedLocation`. If my reasoning is correct then we'd just need
this:

      + InVal.DefInst->applyMergedLocation(InVal.DefInst->getDebugLoc().get(),
      +                                    Inst.getDebugLoc());
        if (!Inst.use_empty())
          Inst.replaceAllUsesWith(Op);
        salvageKnowledge(&Inst, &AC);
        salvageKnowledge(&Inst, &AC);
        removeMSSA(Inst);
  	  

This will give the instruction line 0 for your reproducer. I know a lot less about line numbers than
variable locations, so I'd want someone else to look at that first. @probinson, @jmorse does it
look like that's the correct thing to do here?

Either way, looking a little closer at the ticket, other than the line number issue mentioned above
(which I'm not confident about), I don't think behaviour in the ticket is actually a bug. The
debugger step line (copied below) shows that we're stopping on column 20 of line 6, which is on the
right hand side of the first '=', and IIUC it's reasonable to expect l_52 to no longer be 0 here.

  Copied from https://bugs.llvm.org/show_bug.cgi?id=46114
  Process 2616 stopped
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
      frame #0: 0x0000000000400486 a.out`main at abc.c:6:20
     3   	char c;
     4   	int main() {
     5   	  int d = a, l_52 = 0;
  -> 6   	  b = (l_52 = a) - c; //   stop here
     7   	  if (d) {
     8   	    int e=1;
     9   	  }
  
  (lldb) p l_52
  (int) $0 = 1


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

https://reviews.llvm.org/D80691