Page MenuHomePhabricator

Let CorrelatedValuePropagation preserve LazyValueInfo
ClosedPublic

Authored by aqjune on Mar 14 2019, 1:35 AM.

Details

Summary

This patch makes CorrelatedValuePropagation preserve LazyValueInfo by adding LazyValueInfo::eraseValue & calling it whenever an instruction is erased.

Passes make check , test-suite, and SPECrate 2017.

Patch by aqjune (Juneyoung Lee)

Diff Detail

Repository
rL LLVM

Event Timeline

aqjune created this revision.Mar 14 2019, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 1:35 AM
reames added inline comments.Mar 15 2019, 11:49 AM
lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
122 ↗(On Diff #190573)

I'm confused why we need this. LVI uses ValueHandles, which should delete the entry from the cache once it's destroyed. Do you have a test case where that is not happening?

815 ↗(On Diff #190573)

Have you checked the history? I believe that CVP used to preserve LVI. Why was that removed?

reames requested changes to this revision.Mar 15 2019, 11:51 AM

Marking to remove from review queue. When replying to questions, please mark as ready for review.

This revision now requires changes to proceed.Mar 15 2019, 11:51 AM
aqjune updated this revision to Diff 192022.Mar 23 2019, 9:39 PM

Removes eraseValue() calls

aqjune marked 3 inline comments as done.Mar 23 2019, 9:45 PM
aqjune added inline comments.
lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
122 ↗(On Diff #190573)

You were right, I checked that these items were automatically removed from LazyValueInfoCache. I removed eraseValue() calls.

815 ↗(On Diff #190573)

I checked that LazyValueAnalysis had not been added in the past. DominatorTreeAnalysis was added at https://reviews.llvm.org/D48059 , GlobalsAA was added at https://llvm.org/svn/llvm-project/llvm/trunk@274705 .

aqjune marked an inline comment as done.Mar 23 2019, 10:04 PM

I couldn't find ready for review option from anywhere :( 'Add Action...' dropdown menu contains Plan Changes and Abandon Revision only.

reames accepted this revision.Mar 26 2019, 6:13 PM

LGTM. This may be slightly imprecise - for instance, when we fold an operand which allows further LVI analysis - but the preservation is conservatively correct.

p.s. Updating a review resets the status.

This revision is now accepted and ready to land.Mar 26 2019, 6:13 PM

Thanks! :)
I don't have commit access; if you don't mind, could you commit this patch instead please?

Please rebase

aqjune updated this revision to Diff 211126.Mon, Jul 22, 9:23 AM

Sorry, I was pretty busy last week.
@xbolva00 Rebased

xbolva00 edited the summary of this revision. (Show Details)Wed, Jul 24, 1:26 PM
This revision was automatically updated to reflect the committed changes.

Sorry, I was pretty busy last week.
@xbolva00 Rebased

Commited, r366942.