Page MenuHomePhabricator

[DSE] Eliminate stores by terminators (free,lifetime.end).
ClosedPublic

Authored by fhahn on Jan 8 2020, 12:49 PM.

Details

Summary

This patch adds support for eliminating stores by free & lifetime.end
calls. We can remove stores that are not read before calling a memory
terminator and we can eliminate all stores after a memory terminator
until we see a new lifetime.start. The second case seems to not really
trigger much in practice though.

Diff Detail

Event Timeline

fhahn created this revision.Jan 8 2020, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2020, 12:49 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

fhahn planned changes to this revision.Jan 17 2020, 9:32 AM

Needs updating to work bottom-up.

fhahn updated this revision to Diff 243566.Feb 10 2020, 8:02 AM

Ping. Rebased after D72700 landed.

fhahn updated this revision to Diff 273340.Thu, Jun 25, 6:54 AM

Ping :) Rebased after recent changes.

asbirlea added inline comments.Tue, Jul 7, 1:03 PM
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1705

I'm not sure how expensive the isFreeCall is, but the check is already done inside the call to getLocForTerminator. You could add a bool optional, or return a pair, if worth eliminating the repeated check.

2268

I don't understand how this change is related.

fhahn updated this revision to Diff 276200.Tue, Jul 7, 1:40 PM

Addressed comments, thanks! I updated getLocForTerminator to return an optional pair and removed the unnecessary ToCheck.insert calls.

fhahn marked 3 inline comments as done.Tue, Jul 7, 1:41 PM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
1705

Done, thanks!

2268

It looks like this was a leftover from an earlier rebase. I removed it. It should not be required as it is already added around line 2205.

asbirlea accepted this revision.Tue, Jul 7, 4:31 PM

Thanks you, LGTM.

This revision is now accepted and ready to land.Tue, Jul 7, 4:31 PM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.