Page MenuHomePhabricator

[OPT_DPG][LIVEDEBUGVALUES] Teach Live Debug Values About Meta Instructions

Authored by TWeaver on Jan 22 2020, 9:05 AM.



Previously LiveDebugValues pass would consider meta instructions that 'fiddle' with liveness of registers as register definitions when transfering register defs. This would mean that, for example, a KILL instruction would cause LiveDebugValues to terminate the range of an earlier DBG_VALUE instruction resulting in the none propogation of said DBG_VALUE instructions into later blocks.

This patch adds the check and a helpful comment, fixes a test that previously tested for the broken behaviour by coincidence and adds a test specifically for this.

Thanks for your time reviewing!

Diff Detail

Event Timeline

TWeaver created this revision.Jan 22 2020, 9:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 9:05 AM
djtodoro added inline comments.

What is the intention here?

TWeaver marked an inline comment as done.Jan 22 2020, 9:49 AM
TWeaver added inline comments.

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.

Tom W

vsk accepted this revision.Jan 22 2020, 11:33 AM

LGTM, but please wait for another +1.


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.

This revision is now accepted and ready to land.Jan 22 2020, 11:33 AM
djtodoro added inline comments.Jan 23 2020, 1:12 AM

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)

I missed adding you as a reviewer @djtodoro , my apologies. This has been rectified.

TWeaver updated this revision to Diff 239886.Jan 23 2020, 6:57 AM
TWeaver marked an inline comment as done.

KILL instruction is no reinstated and failing check line has been removed.

vsk added inline comments.Jan 23 2020, 9:36 AM

Seems fine to me.

(Just minor comments about tags used in the to-be (?) commit message.)

There is a typo in [OPT_DPG]. OPT[_-]DBG seems to be uncommon. I'd personally prefer [DebugInfo] as that is an established tag. I'd also prefer writing LIVEDEBUGVALUES in camel case instead: [LiveDebugValues].

TWeaver added inline comments.Jan 23 2020, 10:14 AM

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

Hi David,

thanks for your review and for spotting out the typos.

I'll ensure to use the correct tags from here on out and of course use the camel case spelling for LiveDebugValues.

I'll probably land this tomorrow as it's home time soon and I'd like to avoid being potentially pinged by build bots all evening.

thanks again
Tom W

djtodoro accepted this revision.Jan 23 2020, 11:23 PM
djtodoro added inline comments.

Good! Thanks!

This revision was automatically updated to reflect the committed changes.