[PATCH] D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations

Wed Mar 27 03:03:15 PDT 2019

bjope added a comment.

I'll try to describe one "regression" that I've seen.

Before ISel we've got

  ...
  %0 = tail call %struct. @llvm.phx.divm.u16.s_struct.s(i16 %a, i16 %b), !dbg !708
  %.fca.0.extract = extractvalue %struct. %0, 0, !dbg !708
  call void @llvm.dbg.value(metadata i16 %.fca.0.extract, metadata !686, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 16)), !dbg !709
  %.fca.1.extract = extractvalue %struct. %0, 1, !dbg !708
  call void @llvm.dbg.value(metadata i16 %.fca.1.extract, metadata !686, metadata !DIExpression(DW_OP_LLVM_fragment, 16, 16)), !dbg !709
  ...
  use %.fca.0.extract
  use %.fca.1.extract
  ...

After ISel it looks like this

  ...
  %14:anh_0_7, %15:anh_0_7 = divm16_pseudo killed %13:anh_0_7, %7:anh_0_7, implicit-def dead $ccreg, debug-location !708; 
  DBG_VALUE %14:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
  %0:anh_rn = COPY %15:anh_0_7, debug-location !708;
  DBG_VALUE %15:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
  ...
  use %14:anh_0_7
  use %0:anh:rn
  ...

//Maybe ISel should have put the COPY to %0 after the last DBG_VALUE (now we got a dbg-use of %15 after the last non-dbg-use of %15). Or maybe it should have used %0 instead of %15 in that DBG_VALUE. Or maybe there should be one DBG_VALUE before the COPY using %15 and one after using %0.//

Before simple register coalescing we get this (some more COPY instructions after expanding the pseudo that is using some hard coded physical registers):

  ...
  208B	  divm16 0, $noreg, 0, implicit-def $a4_40, implicit-def dead $af4, implicit-def dead $ccreg, implicit-def $a5_40, implicit $a4h, implicit $a5h, debug-location !708;
  224B	  %42:an40_0_7 = COPY $a4_40, debug-location !708;
  240B	  %14:anh_0_7 = COPY %42.hi16:an40_0_7, debug-location !708;
  256B	  %43:an40_0_7 = COPY $a5_40, debug-location !708;
  272B	  %15:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
  	  DBG_VALUE %14:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
  288B	  %0:anh_0_7 = COPY %15:anh_0_7, debug-location !708;
  	  DBG_VALUE %15:anh_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
  ...
            use %14:anh_0_7
            use %0:anh_0_7
  ...

Without this patch we get

  ...
  208B	  divm16 0, $noreg, 0, implicit-def $a4_40, implicit-def dead $af4, implicit-def dead $ccreg, implicit-def $a5_40, implicit $a4h, implicit $a5h, debug-location !708;
  224B	  %42:an40_0_7 = COPY $a4_40, debug-location !708;
  256B	  %43:an40_0_7 = COPY $a5_40, debug-location !708;
  	  DBG_VALUE %42.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
  	  DBG_VALUE %43.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
  ...
            use %42.hi16:an40_0_7
            use %43.hi16:an40_0_7
  ...

With the patch the last DBG_VALUE is changed into using %noreg, so instead we get

  ...
  208B	  divm16 0, $noreg, 0, implicit-def $a4_40, implicit-def dead $af4, implicit-def dead $ccreg, implicit-def $a5_40, implicit $a4h, implicit $a5h, debug-location !708;
  224B	  %42:an40_0_7 = COPY $a4_40, debug-location !708;
  256B	  %43:an40_0_7 = COPY $a5_40, debug-location !708;
  	  DBG_VALUE %42.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !709;
  	  DBG_VALUE %noreg.hi16, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
  ...
            use %42.hi16:an40_0_7
            use %43.hi16:an40_0_7
  ...

It is when analyzing the COPY to %0 that we detect that the DBG_VALUE is unsound:

  272B	%15:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
  	Considering merging to aN40_0_7 with %15 in %43:hi16
  		RHS = %15 [272r,288r:0)  0 at 272r weight:0.000000e+00
  		LHS = %43 [256r,272r:0)  0 at 256r weight:0.000000e+00
  		merge %15:0 at 272r into %43:0 at 256r --> @256r
  		erased:	272r	%15:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
  		updated: 288B	%0:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
  		updated: DBG_VALUE %43.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
  Shrink: %43 [256r,288r:0)  0 at 256r weight:0.000000e+00
  Shrunk: %43 [256r,288r:0)  0 at 256r weight:0.000000e+00
  	Success: %15:hi16 -> %43
  	Result = %43 [256r,288r:0)  0 at 256r weight:0.000000e+00

  288B	%0:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
  	Considering merging to aN40_0_7 with %0 in %43:hi16
  		RHS = %0 [288r,928r:0)  0 at 288r weight:0.000000e+00
  		LHS = %43 [256r,288r:0)  0 at 256r weight:0.000000e+00
  		merge %0:0 at 288r into %43:0 at 256r --> @256r
  		erased:	288r	%0:anh_0_7 = COPY %43.hi16:an40_0_7, debug-location !708;
  		updated: 928B	%54:anh_0_7 = subs_a16_a16_a16 %40:anh_0_7, %43.hi16:an40_0_7, 0, $noreg, 0, implicit-def dead $ccreg, implicit $cuc, debug-location !758;
  Update of %43.hi16:an40_0_7 would be unsound, setting undef

So we have already merged  `aN40_0_7 with %15 in %43:hi16` and updated the DBG_VALUE to use %43:hi16 before we find it unsound. So in the past this coalescing made the DBG_VALUE sound (as it extended the live range for %43 to cover the DBG_VALUE). But with this patch we instead detect the DBG_VALUE as being unsound.

Inside mergingChangesDbgValue I can see

  DbgV->dump() =>   DBG_VALUE %43.hi16:an40_0_7, $noreg, !"cycleDiv", !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !709;
  DbgReg = 2147483691 (%43)
  DstReg = 2147483691 (%43)
  SrcReg = 2147483648 (%0)
  SrcLive = true
  DstLive = false

That is basically how far I've analysed this scenario at the moment.

Some thoughts:

- Should this be seen as a fault in ISel? We could probably get a better result if ISel worked differently. But in general I think we allow dbg-uses outside the non-dbg-live-interval, so I do not think it really is a fault.
- A late dbg-use (for an otherwise killed reg) is only unsound if the register allocator is using the allocated physical register for some other purpose. Isn't it a little bit early to detect that in the register coalescer? Maybe we need to take the register allocation into account if we want to make a better solution. Isn't LiveDebugVariables supposed to handle this?
- Is this patch really supposed to detect this DBG_VALUE as unsound? (we are not updating %43 here, we are only extending the live range to cover the DBG_VALUE that already is using %43). Should perhaps mergingChangesDbgValue return false also when `(SrcLive && DstReg == DbgReg)`, or could this be unsound?

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

https://reviews.llvm.org/D56151