[PATCH] D80691: Proposed fix for PR46114

[Qirun Zhang via Phabricator via llvm-commits]

helloqirun added a comment.

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

> 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


@Orlando I think you are probably right. It should indeed prints `$0=1`. I appreciate your patience and my apology for the report. Shall I close the original PR?


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

https://reviews.llvm.org/D80691