[PATCH] D73210: [OPT_DPG][LIVEDEBUGVALUES] Teach Live Debug Values About Meta Instructions

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro accepted this revision.
djtodoro added inline comments.


================
Comment at: llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir:106
     DBG_VALUE $ebx, $noreg, !17, !DIExpression(), debug-location !20
-    renamable $edi = KILL $edi, implicit-def $rdi
+    $edi = MOV32ri 5, debug-location !22
     DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !20
----------------
vsk wrote:
> TWeaver wrote:
> > djtodoro wrote:
> > > vsk wrote:
> > > > TWeaver wrote:
> > > > > djtodoro wrote:
> > > > > > What is the intention here?
> > > > > The KILL was being picked up as a def in the test and was causing a DBG_VALUE instruction to be created. This was tested for in the check list. Ignoring KILL instructions means that livedebugvalues wasn't creating the second entry DBG_VALUE for ARG_A anymore. 
> > > > > 
> > > > > In order to further preserve what I figured was intended behaviour in the test I changed it to a concrete register def.
> > > > > 
> > > > > The original entry value patch, SFAIK, had nothing to do with KILL/meta instructions. I figured the KILL instruction was there by coincidence rather than intention.
> > > > > 
> > > > > happy to reconsider this if that's not the case.
> > > > > 
> > > > > Cheers
> > > > > Tom W
> > > > This seems like a reasonable change, we just want a real def here. We could just remove the second CHECK for ARG_A, but that arguably drops code coverage.
> > > I see the result, but I don't think we should change the code generated here, since it was produced from the C code provided in the source code of the test. Now we have the two real identical defs for the `$edi`..
> > > Anyhow, the main intention in the test was to test the `ARG_B` doesn't have the `entry_val` generated, since its entry value gets modified, and it seems reasonable to me to just remove the first `entry_val` check for the `ARG_A`.
> > > @vsk Sounds reasonable?
> > > 
> > > @TWeaver Thanks for this! Besides this, lgtm! (but I am not listed as a reviewer)
> > great, that works for me.
> > 
> > It was always a case of remove the check or change the instructions.
> > 
> > happy to have gotten a more definitive decision on which one was the best approach.
> > 
> > I'll get this changed out re-uploaded. an official LGTM would be great and I'll get his pushed today.
> > 
> > thanks again!
> > Tom W
> Seems fine to me.
Good! Thanks!


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

https://reviews.llvm.org/D73210