This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][20/*] Account for assignment tracking in DSE
ClosedPublic

Authored by Orlando on Sep 5 2022, 8:27 AM.

Details

Summary

DeadStoreElimmination shortens stores that are shadowed by later stores such that the overlapping part of the store is omitted. Insert an unlinked dbg.assign intrinsic with a variable fragment that describes the omitted part to signal that that fragment of the variable has a stale value in memory.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:27 AM
Orlando requested review of this revision.Sep 5 2022, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:27 AM
Orlando retitled this revision from [Assignment Tracking][20/*] Account for assignment tracking in ADCE to [Assignment Tracking][20/*] Account for assignment tracking in DCE.Sep 6 2022, 12:21 AM
Orlando retitled this revision from [Assignment Tracking][20/*] Account for assignment tracking in DCE to [Assignment Tracking][20/*] Account for assignment tracking in DSE.Sep 6 2022, 12:41 AM
jmorse added a subscriber: jmorse.Sep 9 2022, 7:43 AM

If I understand the dse-after-memcpyopt-merge.ll test, the stores after the memset get deleted, because they're effectively done by the memset. However: don't we get the right behaviour with no modifications? There's an assignment of zero to those two fields, and the dbg.assigns record those facts even after the stores are deleted. The location list computed at the end of compilation can rightly say those fields are zero, no?

Same with the second test -- the optimisation is shortening the amount of storing happening, but it isn't optimising away the assignment to those fields, just re-implementing them.

To put it another way -- why is it necessary for those fields to be undef after the shortening of the overlapping stores?

llvm/test/DebugInfo/Generic/assignment-tracking/dse/dse-after-memcpyopt-merge.ll
11–16

Probably no need to put a history in the test comment; it isn't a past behaviour that anyone is going to observe, right?

Orlando updated this revision to Diff 459741.Sep 13 2022, 7:01 AM
Orlando marked an inline comment as done.
Orlando edited the summary of this revision. (Show Details)

If I understand the dse-after-memcpyopt-merge.ll test, the stores after the memset get deleted, because they're effectively done by the memset. However: don't we get the right behaviour with no modifications? There's an assignment of zero to those two fields, and the dbg.assigns record those facts even after the stores are deleted. The location list computed at the end of compilation can rightly say those fields are zero, no?

Same with the second test -- the optimisation is shortening the amount of storing happening, but it isn't optimising away the assignment to those fields, just re-implementing them.

To put it another way -- why is it necessary for those fields to be undef after the shortening of the overlapping stores?

Good question (another! I very much appreciate this level of review, thank you!). The value component shouldn't need to be undef, but this one is slightly odd because I chose to link the inserted undef dbg.assign to the memset. I think my original intention was to make it "obvious" that the dbg.assign marked the assignment as not-in-memory despite retaining the link. However, a) I'm not convinced that retaining the link is the right thing to do, and b) an undef address component should be enough to signal that the memory location is stale for this fragment if it were.

Yes, thinking about it more, I think we should not retain the DIAssignID link and the value component shouldn't be undef'd. Updated.

jmorse accepted this revision.Sep 14 2022, 8:07 AM

LGTM

llvm/test/DebugInfo/Generic/assignment-tracking/dse/dse-after-memcpyopt-merge.ll
11
This revision is now accepted and ready to land.Sep 14 2022, 8:07 AM
This revision was landed with ongoing or failed builds.Nov 15 2022, 5:43 AM
This revision was automatically updated to reflect the committed changes.
Orlando marked an inline comment as done.