Page MenuHomePhabricator

ARM64: Debug info for structure argument missing DW_AT_location
ClosedPublic

Authored by kamleshbhalui on Jan 21 2020, 8:21 PM.

Diff Detail

Event Timeline

kamleshbhalui created this revision.Jan 21 2020, 8:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2020, 8:21 PM
vsk added inline comments.Jan 22 2020, 9:22 AM
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
178

If MI is erased, why should its debug uses not be made undef? What really causes those debug uses to be updated correctly?

kamleshbhalui added a comment.EditedJan 22 2020, 10:29 AM

@vsk thanks for reviewing.

below dump is taken, by debugging the clang with this test

#include<stdio.h>
typedef struct {
double a,b,c,d,e;
}mystruct;

void foo(mystruct ms){
printf("%llu\n",(unsigned long long) &ms);
}

This is the dump before erasing the MI ()

bb.1.entry:
  liveins: $x0
  %0:gpr64(p0) = COPY $x0
  %4:gpr64(p0) = MOVaddr target-flags(aarch64-page) @.str, target-flags(aarch64-pageoff, aarch64-nc) @.str, debug-location !DILocation(line: 0, scope: !9); kk3.c:0
  DBG_VALUE %0:gpr64(p0), $noreg, !"ms", !DIExpression(DW_OP_deref), debug-location !22; kk3.c:5:19 line no:5
  %1:gpr64(s64) = COPY %0:gpr64(p0), debug-location !23; kk3.c:7:1
  ADJCALLSTACKDOWN 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  $x0 = COPY %4:gpr64(p0), debug-location !24; kk3.c:6:5
  $x1 = COPY %1:gpr64(s64), debug-location !24; kk3.c:6:5
  BL @printf, <regmask $fp $lr $b8 $b9 $b10 $b11 $b12 $b13 $b14 $b15 $d8 $d9 $d10 $d11 $d12 $d13 $d14 $d15 $h8 $h9 $h10 $h11 $h12 $h13 $h14 $h15 $s8 $s9 $s10 $s11 $s12 $s13 $s14 and 51 more...>, implicit-def $lr, implicit $sp, implicit $x0, implicit $x1, implicit-def $w0, debug-location !24; kk3.c:6:5
  ADJCALLSTACKUP 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  RET_ReallyLR debug-location !25; kk3.c:8:1

Once MI ( %1:gpr64(s64) = COPY %0:gpr64(p0), debug-location !23; kk3.c:7:1)
is getting erased based on condition at line 176 and It is marked the DBG_VALUE for removal.
This is the dump

# *** IR Dump After InstructionSelect ***:
# Machine code for function foo: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected
Function Live Ins: $x0

bb.1.entry:
  liveins: $x0
  %0:gpr64 = COPY $x0
  %4:gpr64 = MOVaddr target-flags(aarch64-page) @.str, target-flags(aarch64-pageoff, aarch64-nc) @.str, debug-location !DILocation(line: 0, scope: !9); kk3.c:0
  DBG_VALUE $noreg, $noreg, !"ms", !DIExpression(DW_OP_deref), debug-location !22; kk3.c:5:19 line no:5
  ADJCALLSTACKDOWN 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  $x0 = COPY %4:gpr64, debug-location !24; kk3.c:6:5
  $x1 = COPY %0:gpr64, debug-location !24; kk3.c:6:5
  BL @printf, <regmask $fp $lr $b8 $b9 $b10 $b11 $b12 $b13 $b14 $b15 $d8 $d9 $d10 $d11 $d12 $d13 $d14 $d15 $h8 $h9 $h10 $h11 $h12 $h13 $h14 $h15 $s8 $s9 $s10 $s11 $s12 $s13 $s14 and 51 more...>, implicit-def $lr, implicit $sp, implicit $x0, implicit $x1, implicit-def $w0, debug-location !24; kk3.c:6:5
  ADJCALLSTACKUP 0, 0, implicit-def $sp, implicit $sp, debug-location !24; kk3.c:6:5
  RET_ReallyLR debug-location !25; kk3.c:8:1

Which makes difficult to debug even unoptimized code.

kamleshbhalui marked an inline comment as done.Jan 22 2020, 10:45 AM
kamleshbhalui added inline comments.
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
178

since it is unoptimized it should not mark debug uses as undef.

vsk added inline comments.Jan 22 2020, 11:25 AM
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
178

I'm not sure there's something special about unoptimized code here. GIsel seems to eliminate redundant copies even at -O0 (maybe to reduce compile time).

Oh, I think I see what's happening now. When GISel sees a redundant copy, it deletes it /after/ replacing vreg uses of DstReg with SrcReg, at which point valid debug uses of SrcReg are accidentally deleted.

Prior to the replaceRegWith call, are all debug uses of DstReg guaranteed to be dominated by the def of SrcReg? If so, could you leave a comment explaining why?

If not, for this particular test case, I'd expect that swapping the lines MRI.replaceRegWith(DstReg, SrcReg); and MI.eraseFromParentAndMarkDBGValuesForRemoval(); would also cause the test to pass.

dsanders added inline comments.Jan 22 2020, 12:34 PM
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
178

since it is unoptimized it should not mark debug uses as undef.

Whether it's optimized or not isn't really the important factor there. It's whether the data remains valid (or is repairable) or not. Having looked at your test case, I don't see a reason why the data is invalid so we should keep it.

I'm not sure there's something special about unoptimized code here. GIsel seems to eliminate redundant copies even at -O0 (maybe to reduce compile time).

It's partly compile time and partly because they block matches that span multiple MI's.

If not, for this particular test case, I'd expect that swapping the lines MRI.replaceRegWith(DstReg, SrcReg); and MI.eraseFromParentAndMarkDBGValuesForRemoval(); would also cause the test to pass.

Are you sure? I think it would have the same effect as it's the MI.eraseFromParentAndMarkDBGValuesForRemoval() would still remove the %1 = COPY ... and the DBG_VALUE %0, ... and then replace the remaining %1's with %0. This would update the DBG_VALUE %0 to DBG_VALUE %1 but it would then be deleted anyway as it's already been scheduled.

Prior to the replaceRegWith call, are all debug uses of DstReg guaranteed to be dominated by the def of SrcReg? If so, could you leave a comment explaining why?

I'm not convinced that's guaranteed but i also doubt it matters. The effect would be to bind multiple source variables to the same register and I believe that's ok.

kamleshbhalui marked an inline comment as done.Jan 23 2020, 5:04 AM
kamleshbhalui added inline comments.
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
178

Yes reordering this two statements

MRI.replaceRegWith(DstReg, SrcReg); MI.eraseFromParentAndMarkDBGValuesForRemoval();

to

MI.eraseFromParentAndMarkDBGValuesForRemoval();
MRI.replaceRegWith(DstReg, SrcReg);

solves the problems(preserve the dbg_value).

kamleshbhalui edited the summary of this revision. (Show Details)

reversed the ordering.

dsanders added inline comments.Jan 23 2020, 3:13 PM
llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp
178

I've tried it now and I can see that it does work for this test case but not for a slight variation of it. If the input contains DEBUG_VALUE %1 as in

then the original code produces:

DBG_VALUE $noreg, $noreg, !21, !DIExpression(DW_OP_deref), debug-location !22
DBG_VALUE $noreg, $noreg, <0x7f9725c2ba20>, !DIExpression(DW_OP_deref), debug-location !22

(the hex address there is just because the DILocalVariable isn't referenced in the LLVM-IR), losing both %0 and %1's DBG_VALUE. The reordered code produces:

DBG_VALUE %0, $noreg, !21, !DIExpression(DW_OP_deref), debug-location !22
DBG_VALUE $noreg, $noreg, <0x7fae7b42ba20>, !DIExpression(DW_OP_deref), debug-location !22

preserving the %0 one but still losing %1's. Meanwhile your original patch produces:

DBG_VALUE %0, $noreg, !21, !DIExpression(DW_OP_deref), debug-location !22
DBG_VALUE %0, $noreg, <0x7ff15dd21d40>, !DIExpression(DW_OP_deref), debug-location !22

which looks the most correct to me

vsk added a comment.Jan 23 2020, 3:35 PM

Thanks Daniel. If it's safe not to mark dbg values for removal here, that seems like the best option.

rebased and updated diff to original patch.
Thanks @vsk and @dsanders for reviewing.

This revision is now accepted and ready to land.Jan 24 2020, 10:43 AM

I do not have commit access.
can someone commit this?

dstenb added a subscriber: dstenb.Jan 28 2020, 6:56 AM

I do not have commit access.
can someone commit this?

I don't have time for the build bots today, but I can commit it tomorrow morning (European time) unless someone else picks it up before.

This revision was automatically updated to reflect the committed changes.