[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();
         }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73159