Page MenuHomePhabricator

helloqirun (Qirun Zhang)
User

Projects

User does not belong to any projects.

User Details

User Since
May 27 2020, 5:51 PM (10 w, 3 d)

Recent Activity

Jun 2 2020

helloqirun added a comment to D80691: Proposed fix for PR46114.

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?

Jun 2 2020, 7:08 AM · debug-info, Restricted Project

Jun 1 2020

helloqirun added a comment to D80691: Proposed fix for PR46114.

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).

Jun 1 2020, 5:53 PM · debug-info, Restricted Project

May 31 2020

helloqirun updated the diff for D80691: Proposed fix for PR46114.

I have added the test case and made a minor change. The RemoveRedundantDbgInstrs call will always keep the second dbg.value. 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. I think @Orlando's suggestion on merging the locations will be a good improvement.

May 31 2020, 11:10 AM · debug-info, Restricted Project

May 29 2020

helloqirun added a comment to D80691: Proposed fix for PR46114.

I think that this patch reduces variable coverage by undefing dbg.values which
it cannot salvage that would otherwise be RAUWd.

I'd expect your call to salvageDebugInfoOrMarkUndef to insert an 'undef'
dbg.value for 'l_52' for your original reproducer because you cannot salvage a
load operation. When I build with this patch locally, that is what I get. lldb
(and gdb) tells me that the value for 'l_52' has been optimized out.

With your patch applied, using your original reproducer:

$ 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-with-your-patch -O3 -g test.c
$ lldb a.out
...
(lldb) s
Process 21970 stopped
* thread #1, name = 'a.out', stop reason = step in
    frame #0: 0x0000000000400486 a.out`main at test.c:6:20
   3   	char c;
   4   	int main() {
   5   	  int d = a, l_52 = 0;
-> 6   	  b = (l_52 = a) - c; //   qirun0
   7   	  if (d) {
   8   	    int e=1;
   9   	  }
(lldb) p l_52
error: Couldn't materialize: couldn't get the value of variable l_52: no location, value may have been optimized out

My lldb is built at 9b56cc9361a471a3ce57da4c98f60e377cc43026 (1st April 2020),
and clang 709c52b9553 (18th May 2020). What do you see in lldb when you build
your original reproducer with this patch?

There are cases where EarlyCSE needs to remove the first (the following case). The instruction %0 = load i32, i32* @a, align 4 can be the product of both EarlyCSE DCE (the following case) and EarlyCSE CSE LOAD (the case in the original PR).

My understanding is that DCE and CSE do not have equal implications for
debug-info. DCE means the value is going away because it isn't used. In this
case we should salvage any debug users (i.e. dbg.values using the value),
because the value will no longer exist. For CSE the value still exists
somewhere, so we instead want to update debug users to use the value which we
are keeping. This is what the line Inst.replaceAllUsesWith(Op); is doing
without your patch. However, with your patch applied, the salvage call replaces
the dbg.values with a constant if it can salvage, or undef if it cannot. This
means that the replaceAllUsesWith (RAUW) cannot update the debug users.

Thanks! Yes, if we want to have two dbg.value in the IR, it works fine. That's precisely what was before e5f07080b8a, 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.

I wouldn't expect this to result in you seeing a value of '0' in the debugger.

May 29 2020, 4:57 PM · debug-info, Restricted Project
helloqirun added a comment to D80691: Proposed fix for PR46114.

Please add a test as well.

May 29 2020, 4:57 PM · debug-info, Restricted Project

May 28 2020

helloqirun updated the diff for D80691: Proposed fix for PR46114.

Re-formatted the comment.

May 28 2020, 11:34 AM · debug-info, Restricted Project
helloqirun added a comment to D80691: Proposed fix for PR46114.

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?

May 28 2020, 7:02 AM · debug-info, Restricted Project
helloqirun added a comment to D80691: Proposed fix for PR46114.

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, 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.

May 28 2020, 6:30 AM · debug-info, Restricted Project

May 27 2020

helloqirun updated the summary of D80691: Proposed fix for PR46114.
May 27 2020, 11:57 PM · debug-info, Restricted Project
helloqirun created D80691: Proposed fix for PR46114.
May 27 2020, 11:24 PM · debug-info, Restricted Project