[PATCH] D73159: ARM64: Debug info for structure argument missing DW_AT_location

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp:178
           MRI.replaceRegWith(DstReg, SrcReg);
-          MI.eraseFromParentAndMarkDBGValuesForRemoval();
+          MI.eraseFromParent();
         }
----------------
kamleshbhalui wrote:
> dsanders wrote:
> > vsk wrote:
> > > kamleshbhalui wrote:
> > > > vsk wrote:
> > > > > If MI is erased, why should its debug uses not be made undef? What really causes those debug uses to be updated correctly?
> > > > since it is unoptimized it should not mark debug uses as undef.
> > > > 
> > > 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.
> > > 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.
> 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).
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 {F11239786} 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


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

https://reviews.llvm.org/D73159