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

[kamlesh kumar via Phabricator via llvm-commits]

kamleshbhalui marked an inline comment as done.
kamleshbhalui added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/InstructionSelect.cpp:178
           MRI.replaceRegWith(DstReg, SrcReg);
-          MI.eraseFromParentAndMarkDBGValuesForRemoval();
+          MI.eraseFromParent();
         }
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73159